gpertea / stringtie

Transcript assembly and quantification for RNA-Seq
MIT License
378 stars 78 forks source link

Respect CC/CXX/CFLAGS/CXXFLAGS values #185

Open mmokrejs opened 6 years ago

mmokrejs commented 6 years ago

Hi, please consider most of the changes in https://github.com/gentoo/sci/blob/master/sci-biology/stringtie/files/Makefile.patch . Except the first 3 lines which should be more general:

BAM_INC = -I/usr/include/bam
BAM_LIB =  # or -L/some/path
BAM_LIBS = -lbam

To explain, Gentoo Linux installs old samtools-0.x series headers in a separate location (and the library under a different name), to allow simultaneous installation with never samtools-1.x. Otherwise I believe the patch can be accepted. Other software packagers will appreciate you do not ignore their environment settings.

gpertea commented 6 years ago

No -- I respectfully disagree. The samtools-0.x code included here actually differs from any "official" samtools-0.x distribution, as I fixed and/or back-ported some patches from the newer 1.x releases of the library. It's really a private, customized copy of samtools 0.x code and should not be installed globally (system-wide). Also, it's meant to be just statically linked during the build process into the stringtie binary so there is really no reason for a system-wide installation anyway. Also, as opposed to my other projects (e.g. gffread, gffcompare etc.) , stringtie has its own copy of the gclib "code library" (again, not a "proper" object library) and as such it is totally self-contained and built in place as is. These private instances of gclib or samtools-0.x should not be "installed" system wide.

Back to samtools: I do plan to switch to the official htslib (samtools-1.x) distribution in the near future, which would indeed then allow setting BAM_* to the system-wide installation. But until then, sorry..

mmokrejs commented 6 years ago

Still, please try to respect user's environment settings in your Makefile. Please reopen until that happens. Gentoo/Debian/Bioconda/etc., all are wasting time on fixing your Makefiles. Thank you.

gpertea commented 6 years ago

Although I disagree with this attitude (as I believe that this kind of software development does not have to be tied to some particular packaging requirements), I am very willing to cooperate, within reasonable limits. The instructions for building this software, as provided here and on the official site, work just fine for anyone trying to build this software from source -- never had any complaints about it, with the source package being self-contained as it is. So I could argue that "fixing" the Makefile is not really an issue here, and I certainly did not intend for anyone to "waste time on fixing" the Makefile -- just because it does not conform to some particular third-party packaging requirements.. However I do agree with adjusting the Makefile as to make it easier for such packaging methods, as long as such adjustments do not alter the current, simple and straight forward build instructions for this software.

mmokrejs commented 6 years ago

Thank you for your efforts. I admit I evidently forgot to check for current version but nevertheless I hope the patches I made are clear in intent. I think you can find similar in Debian repositories too.

gpertea commented 6 years ago

Regarding the Makefile adjustments, it seems that statements like CXX ?= g++ fail when CXX is set to an empty string -- though I agree that is very unlikely for that to ever be the case.. but it might get slightly less unlikely for stuff like CXXFLAGS and LDFLAGS in some wacky environments (?). So is it OK with you (in terms of potential portability issues) if I change that kind of assignment to a conditional instead? Like this:

CXX := $(if $(CXX),$(CXX),g++)

mmokrejs commented 6 years ago

Works fine for me for the main stringtie directory, thank you.

Something similar needs to be done for ./samtools-0.1.18 too. Currently Gentoo uses https://packages.gentoo.org/packages/sci-biology/samtools/files/samtools-0.1.20-buildsystem.patch for the 0.1.20 sources.