Skip to content

try to respect outside CXXFLAGS - #402

Open
bgruening wants to merge 2 commits into
chhylp123:masterfrom
bgruening:patch-1
Open

try to respect outside CXXFLAGS#402
bgruening wants to merge 2 commits into
chhylp123:masterfrom
bgruening:patch-1

Conversation

@bgruening

Copy link
Copy Markdown

@chhylp123 can you maybe try that?

@chhylp123

chhylp123 commented Feb 19, 2023

Copy link
Copy Markdown
Owner

@bgruening I see. The problem is that the new Makefile clearly introduces the CXXFLAGS, which will utilize the CXXFLAGS environment variable of users. If the CXXFLAGS environment variable is set by bioconda, makefile will still be failed?

@chhylp123

Copy link
Copy Markdown
Owner

@bgruening Sorry I mean if users would like to compile hifiasm manually from the source code, they will simiply type make. In this case, makefile will introduce the CXXFLAGS environment variable.

@bgruening

Copy link
Copy Markdown
Author

Yes, you need to find a solution to set all the parameters when a user executes make but this should still respect the ENV vars set by the user externally - in this case, conda.

You could also use VARIABLE ?= value to just set CXXFLAGS when its not set outside.

@chhylp123

Copy link
Copy Markdown
Owner

@bgruening I see, thanks a lot. I will merge it in the next release. As I already cut a release for the 0.18.7, I just want to figure out a way to modify conda receipt, instead of the Makefile. This is also helpful if the users would like to utilize previous versions by bioconda. I'm thinking probably could I directly incorporate -L within CXXFLAGS (although it is not a good solution)?:

make CXXFLAGS="-g -I$PREFIX/include -O3 -msse4.2 -mpopcnt -fomit-frame-pointer -Wall -L$PREFIX/lib" CC=${CC} CXX=${CXX}

@bgruening

Copy link
Copy Markdown
Author

If you are searching for a hack, remove the lines from your Makefile as part of the conda recipe :)

See here for inspiration: https://github.com/bioconda/bioconda-recipes/blob/f7060664019d76f30ec726d1e3981d4ec2cdb336/recipes/vcflib/build.sh#L9

@chhylp123

Copy link
Copy Markdown
Owner

Thanks a lot! Let me update the the makefile and the receipt with the new release.

@bgruening

Copy link
Copy Markdown
Author

@chhylp123 can you ping me when this is released please? Thanks a lot!

@chhylp123

Copy link
Copy Markdown
Owner

@bgruening Sorry for the delay as I was working on some bugs inside hifiasm. As you mentioned on another issue (#392 (comment)):

This means zlib is not found and I guess this is because you are overwriting CXXFLAGS in your Makefile.

Yes, the GUN make has an override behavior (https://ftp.gnu.org/old-gnu/Manuals/make-3.79.1/html_chapter/make_9.html#SEC90), which means the 'CXXFLAGS' inside the makefile will be overrided by that in the command line. I understand this is why the previous conda version of hifiasm is several times slower as -O3 will be overrided by the conda receipt (https://github.com/bioconda/bioconda-recipes/blob/fca9554c95763983bdc9fb06770f956b90967057/recipes/hifiasm/build.sh#L5). I do understand your following modification for the makefile will be helpful to make hifiasm work with exisiting bioconda receipt. But my question is why the following updated conda receipt cannot pass the review of bioconda?

bioconda/bioconda-recipes@8b5b243

This new receipt clearly set all necessary values for 'CXXFLAGS' in the command line, and it should override the 'CXXFLAGS' inside the makefile. Why bioconda cannot find lz?

PS: there is an urgent bug to be fixed in hifiasm, so I will cut a new release first without your modification.

@bgruening

Copy link
Copy Markdown
Author

This new receipt clearly set all necessary values for 'CXXFLAGS' in the command line, and it should override the 'CXXFLAGS' inside the makefile. Why bioconda cannot find lz?

You are hardcoding these values in your Makefile and do not allow the overwrites. It has nothing to do with Conda or Bioconda you can reproduce this locally as well.

You need to define variables with ?= like here https://github.com/dpryan79/libBigWig/blob/master/Makefile#L4 or := depending on what you need.

@bgruening

Copy link
Copy Markdown
Author

I hacked your Makefile on the bioconda side. Could you maybe try version 0.18.8 and see if its faster?

@chhylp123

chhylp123 commented Feb 23, 2023

Copy link
Copy Markdown
Owner

@bgruening Thanks a lot. But as this issue mentioned (weidai11/cryptopp#525), if users set CXXFLAGS by the command line, the GNU make will overwrite CXXFLAGS no matter it is hardcoded in Makefile or not? With the hardcoded Makefile and the previous bioconda receipt (https://github.com/bioconda/bioconda-recipes/blob/fca9554c95763983bdc9fb06770f956b90967057/recipes/hifiasm/build.sh#L5), hifiasm works with the bioconda but loses -O3. Therefore I guess the GNU make will overwrite CXXFLAGS in anyway?

This new receipt clearly set all necessary values for 'CXXFLAGS' in the command line, and it should override the 'CXXFLAGS' inside the makefile. Why bioconda cannot find lz?

You are hardcoding these values in your Makefile and do not allow the overwrites. It has nothing to do with Conda or Bioconda you can reproduce this locally as well.

You need to define variables with ?= like here https://github.com/dpryan79/libBigWig/blob/master/Makefile#L4 or := depending on what you need.

@bgruening

Copy link
Copy Markdown
Author

@chhylp123 mh ... sorry, this week is just crazy. I can not really test it but I guess it would also work if you write:

make CXXFLAGS="-g -I$PREFIX/include -O3 -msse4.2 -mpopcnt -fomit-frame-pointer -Wall" LIBS="-L$PREFIX/lib -lz -lpthread -lm" CC=${CC} CXX=${CXX}

So using your LIBS variable.

@chhylp123

Copy link
Copy Markdown
Owner

@bgruening Thanks a lot! Have a nice weekend~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants