Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rPackages.Rhisat2: fix build #302192

Merged
merged 1 commit into from
May 3, 2024
Merged

rPackages.Rhisat2: fix build #302192

merged 1 commit into from
May 3, 2024

Conversation

Kupac
Copy link
Contributor

@Kupac Kupac commented Apr 6, 2024

Description of changes

Add required commands

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@Kupac Kupac requested a review from jbedo as a code owner April 6, 2024 21:35
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Apr 6, 2024
@b-rodrigues
Copy link
Contributor

Doesn't build on aarch64-linux:

g++: error: unrecognized command-line option '-m64'
g++: error: unrecognized command-line option '-msse2'
make: *** [Makefile:363: hisat2-build-s] Error 1
ERROR: compilation failed for package 'Rhisat2'
* removing '/nix/store/c4p14lc9a2qgj8pzr2xdcnaf44dgmb1c-r-Rhisat2-1.18.0/library/Rhisat2'
error: builder for '/nix/store/vchryn0rxw0jciqq2wh0rs4xf8br96py-r-Rhisat2-1.18.0.drv' failed with exit code 1;
       last 10 log lines:
       > -fno-strict-aliasing -DHISAT2_VERSION="\"2.2.1\"" -DBUILD_HOST="\"localhost\"" -DBUILD_TIME="\"Tue Apr  9 19:51:52 UTC 2024\"" -DCOMPILER_VERSION="\"`/nix/store/49nb1m1ysq0vx31qp94alpmi6ccsq9l3-gcc-wrapper-13.2.0/bin/g++ -v 2>&1 | tail -1`\"" -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE  -DBOWTIE_MM  -DUSESIMDE -DBOWTIE2 -DNDEBUG -Wall -DMASSIVE_DATA_RLCSA \
       > -I. -I third_party \
       > -o hisat2-build-s hisat2_build.cpp \
       > ccnt_lut.cpp ref_read.cpp alphabet.cpp shmem.cpp edit.cpp gfm.cpp reference.cpp ds.cpp multikey_qsort.cpp limit.cpp random_source.cpp tinythread.cpp diff_sample.cpp hisat2_build_main.cpp \
       > -lpthread
       > g++: error: unrecognized command-line option '-m64'
       > g++: error: unrecognized command-line option '-msse2'
       > make: *** [Makefile:363: hisat2-build-s] Error 1
       > ERROR: compilation failed for package 'Rhisat2'
       > * removing '/nix/store/c4p14lc9a2qgj8pzr2xdcnaf44dgmb1c-r-Rhisat2-1.18.0/library/Rhisat2'
       For full logs, run 'nix log /nix/store/vchryn0rxw0jciqq2wh0rs4xf8br96py-r-Rhisat2-1.18.0.drv'.

guess this would need some specific fix for aarch64-linux, but this could be addressed in another PR.

@Kupac
Copy link
Contributor Author

Kupac commented Apr 9, 2024

Nah, we should try to fix it here. Probably those command line options dont make sense on aarch.

@Kupac
Copy link
Contributor Author

Kupac commented Apr 10, 2024

But TBH I don't know how to fix these compiler issues

@b-rodrigues
Copy link
Contributor

I'll give it a shot

@jbedo
Copy link
Contributor

jbedo commented Apr 10, 2024

Makes sense, -m64 and -msse2 are x86 specific. Upstream now support aarch64, you could backport the patch: fmicompbio/Rhisat2@a0f27b0

@b-rodrigues
Copy link
Contributor

b-rodrigues commented Apr 12, 2024

So I was able to get it to work on my Rpi 5 by adding this:

    Rhisat2 = old.Rhisat2.overrideAttrs (attrs: {
      postPatch = ''
        substituteInPlace "src/Makefile" \
          --replace-fail "BITS_FLAG = -m64" "BITS_FLAG =" \
          --replace-fail "SSE_FLAG=-msse2" "SSE_FLAG="
      '';
      env = (attrs.env or { }) // {
        NIX_CFLAGS_COMPILE = attrs.env.NIX_CFLAGS_COMPILE + " -w";
      };
    });



but I don't know:

  • how to just use this on aarch64-linux
  • if this would be the most elegant way of doing it

the NIX_CFLAGS_COMPILE overwrite is because of this warning:

ERROR: compilation failed for package 'Rhisat2'
* removing '/nix/store/9gbixqccpb29hs9h4wchm8007gy917p4-r-Rhisat2-1.18.0/library/Rhisat2'
error: builder for '/nix/store/jpamm9m9dzlavzyby6iswc71vd2h6j3i-r-Rhisat2-1.18.0.drv' failed with exit code 1;
       last 10 log lines:
       > reference.cpp: In member function 'int BitPairReference::getStretch(uint32_t*, size_t, size_t, size_t) const':
       > reference.cpp:522:26: warning: variable 'origBufOff' set but not used [8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wunused-but-set-variable-Wunused-but-set-variable8;;]
       >   522 |                 uint64_t origBufOff = bufOff;
       >       |                          ^~~~~~~~~~
       > reference.cpp:516:14: warning: variable 'binarySearched' set but not used [8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wunused-but-set-variable-Wunused-but-set-variable8;;]
       >   516 |         bool binarySearched = false;
       >       |              ^~~~~~~~~~~~~~
       > make: *** [Makefile:363: hisat2-build-s] Error 1
       > ERROR: compilation failed for package 'Rhisat2'
       > * removing '/nix/store/9gbixqccpb29hs9h4wchm8007gy917p4-r-Rhisat2-1.18.0/library/Rhisat2'
       For full logs, run 'nix log /nix/store/jpamm9m9dzlavzyby6iswc71vd2h6j3i-r-Rhisat2-1.18.0.drv'.

so I followed the advice linked.

> Rhisat2::hisat2_version()
[1] "/nix/store/130gkb8vcj54g4jyp7v00xl1w4qq1cmr-r-Rhisat2-1.18.0/library/Rhisat2/hisat2 version 2.2.1"
[2] "64-bit"                                                                                           
[3] "Built on localhost"                                                                               
[4] "Fri Apr 12 10:22:13 UTC 2024"                                                                     
[5] "Compiler: gcc version 13.2.0 (GCC) "                                                              
[6] "Options: -O3   -funroll-loops -g3 -std=c++11"                                                     
[7] "Sizeof {int, long, long long, void*, size_t, off_t}: {4, 8, 8, 8, 8, 8}"

@b-rodrigues
Copy link
Contributor

I see that there’s also some macOS-specific stuff in the Makefile, for curiosity’s sake, @philipp-baumann could you try to build this on darwin? Might as well fix it completely in one go 😃

@jbedo
Copy link
Contributor

jbedo commented Apr 12, 2024

I think it'd be better to backport the patch, you're essentially recreating it in your nix expression.

@b-rodrigues
Copy link
Contributor

I think it'd be better to backport the patch, you're essentially recreating it in your nix expression.

ok, here it is:

b-rodrigues@3c4e1f8

I don't know how to add this to this PR, I think the easiest is for @Kupac to simply add it?

@Kupac
Copy link
Contributor Author

Kupac commented Apr 13, 2024

@ofborg build rPackages.Rhisat2

@Kupac
Copy link
Contributor Author

Kupac commented Apr 14, 2024

OK, now darwin builds have various errors. aarch64-darwin complains about msse2, but didn't you remove that flag on aarch already?

@b-rodrigues
Copy link
Contributor

That's what the patch does but there's two other files that I didn't patch which were changed in the original commit, but it didn't seem to be related

fmicompbio/Rhisat2@a0f27b0

Copy link
Contributor

@jbedo jbedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source the patch from github.

pkgs/development/r-modules/default.nix Outdated Show resolved Hide resolved
fixed for aarch-linux by @b-rodrigues

Co-authored-by: Justin Bedő <[email protected]>
@Kupac
Copy link
Contributor Author

Kupac commented Apr 20, 2024

@ofborg build rPackages.Rhisat2

@Kupac
Copy link
Contributor Author

Kupac commented Apr 20, 2024

@jbedo I can't resolve the changes requested once again. I maybe missing something on the github gui.

@Kupac Kupac requested a review from jbedo April 20, 2024 22:26
@philipp-baumann
Copy link
Contributor

philipp-baumann commented Apr 21, 2024

on aarch64-darwin i get

       > /nix/store/0m04cmy32vs1686pzjqajsh50arbv8m4-clang-wrapper-16.0.6/bin/cc -I"/nix/store/6s73b2138xz0k5xc2w9bf8c5ic74js46-R-4.3.2/lib/R/include" -DNDEBUG -DLIBXML -I/nix/store/05g7i28lm20a7wp3hn5pgmkxwqlxqxzs-libxml2-2.12.5-dev/include/libxml2 -DUSE_EXTERNAL_SUBSET=1 -DROOT_HAS_DTD_NODE=1 -DNO_CHECKED_ENTITY_FIELD=1 -DDUMP_WITH_ENCODING=1 -DUSE_XML_VERSION_H=1 -DXML_ELEMENT_ETYPE=1 -DXML_ATTRIBUTE_ATYPE=1 -DNO_XML_HASH_SCANNER_RETURN=1 -DLIBXML_NAMESPACE_HAS_CONTEXT=1 -DHAVE_R_CETYPE_T=1 -D__XMLSEC_FUNCTION__=__func__ -DXMLSEC_NO_SIZE_T -DXMLSEC_NO_GOST=1 -DXMLSEC_NO_GOST2012=1 -DXMLSEC_DL_LIBLTDL=1 -I/nix/store/mkfw27hp2c5fybgqkgvfh4mib9wp8y1n-xmlsec-1.2.34-dev/include/xmlsec1 -I/nix/store/05g7i28lm20a7wp3hn5pgmkxwqlxqxzs-libxml2-2.12.5-dev/include/libxml2 -I/nix/store/189j4cpdxvrcr0s2841yf2vpirw9l11h-libxslt-1.1.39-dev/include -DXMLSEC_CRYPTO_DYNAMIC_LOADING=1 -DHAVE_XML_WITH_ZLIB=1 -DHAVE_XML_HAS_FEATURE=1 -DUSE_R=1 -D_R_=1  -DHAVE_VALIDITY=1 -DXML_REF_COUNT_NODES=1 -DHAVE_LIBXML_SEC=1 -I. -DLIBXML2=1  -isystem /nix/store/rjgv94p1nnj02v0f9r2jpann9x02b4vw-libcxx-16.0.6-dev/include/c++/v1    -fPIC  -g -O2  -c XMLEventParse.c -o XMLEventParse.o
       > XMLEventParse.c:618:31: error: incompatible function pointer types assigning to 'xmlStructuredErrorFunc' (aka 'void (*)(void *, const struct _xmlError *)') from 'void (void *, xmlErrorPtr)' (aka 'void (void *, struct _xmlError *)') [-Wincompatible-function-pointer-types]
       >      xmlParserHandler->serror = RS_XML(structuredErrorHandler);
       >                               ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       > 1 error generated.
       > make: *** [/nix/store/6s73b2138xz0k5xc2w9bf8c5ic74js46-R-4.3.2/lib/R/etc/Makeconf:191: XMLEventParse.o] Error 1
       > ERROR: compilation failed for package 'XML'
       > * removing '/nix/store/8rhiqrqp3dhshbsh517iqmar6lad6pdf-r-XML-3.99-0.16/library/XML'
       For full logs, run 'nix log /nix/store/v0f32bk3np8day46ghz3fmyi7xs818p6-r-XML-3.99-0.16.drv'.
error: 1 dependencies of derivation '/nix/store/j5893qi32x0wgbkc3nhwpfwz6xihwz7h-r-biomaRt-2.58.0.drv' failed to build
error: 1 dependencies of derivation '/nix/store/6nbc7i0vn6lpik375jb67s0cnd5ddxz1-r-rtracklayer-1.62.0.drv' failed to build
error (ignored): error: cannot unlink '/private/tmp/nix-build-r-igraph-1.6.0.drv-0/igraph': Directory not empty
error: 1 dependencies of derivation '/nix/store/ikrmwgy898lspgp47nz02v0568vam664-r-GenomicFeatures-1.54.1.drv' failed to build
error: 1 dependencies of derivation '/nix/store/sm6lkknv6l0lsadik0syg8x3d4ypqnz3-r-Rhisat2-1.18.0.drv' failed to build

nixpkgs on  fix_Rhisat2 [$?] took 20.2s 

@b-rodrigues
Copy link
Contributor

on aarch64-darwin i get

looks like rPackages.XML needs fixing

@philipp-baumann
Copy link
Contributor

on aarch64-darwin i get

looks like rPackages.XML needs fixing

yeah that one would have quite a bit of impact for macOS builds, still a lot reverse depends to be expected. https://stat.ethz.ch/pipermail/r-package-devel/2024q1/010374.html

@Kupac
Copy link
Contributor Author

Kupac commented Apr 21, 2024

Weird, it's not the same error message as ofborg's. It clearly says * DONE (XML). Are you sure it's related?

@philipp-baumann
Copy link
Contributor

Weird, it's not the same error message as ofborg's. It clearly says * DONE (XML). Are you sure it's related?

rPackages.XML alone does not build on my M2 machine for the code base at this PR stage. Maybe ofborg does not rebuild XML package (maybe updated?)

@Kupac
Copy link
Contributor Author

Kupac commented Apr 23, 2024

I think ofborg should work from the same commit as you, no? In theory, at least. For me, so far, the evaluation log was always identical to ofborg's.

@jbedo jbedo merged commit d2dd486 into NixOS:master May 3, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants