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

Bwa-mem2: fix compilation issue #380725

Merged
merged 3 commits into from
Feb 14, 2025
Merged

Bwa-mem2: fix compilation issue #380725

merged 3 commits into from
Feb 14, 2025

Conversation

apraga
Copy link
Contributor

@apraga apraga commented Feb 9, 2025

On latest unstable, there are several implicit function declarations in safestrlib, which are new errors instead of warnings.
safestrlib was not a seperate package but simply a submodule in this nix packages.

I have separated it into a library and updated it to latest unstable, which essentially fixes the issue.

Also, there are no tests upstream (the test folder is for benchmarking).

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 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.

@apraga
Copy link
Contributor Author

apraga commented Feb 9, 2025

@alxsimon What do you think of this separation ? I'm not 100% sure it compiles all arch as before though.
@GaetanLepage for testing and your help in general :)
Thanks

@apraga apraga marked this pull request as ready for review February 9, 2025 21:52
@alxsimon
Copy link
Contributor

@alxsimon What do you think of this separation ? I'm not 100% sure it compiles all arch as before though. @GaetanLepage for testing and your help in general :) Thanks

Hi, I don't have enough experience with this to have an opinion sorry.
As I'm not using Nix anymore, my name should probably be removed from the maintainer field of this package, feel free to replace mine with yours in this PR for example, or a following one.

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 380725


x86_64-linux

✅ 2 packages built:
  • bwa-mem2
  • safestringlib

aarch64-linux

✅ 1 package built:
  • safestringlib

x86_64-darwin

❌ 2 packages failed to build:
  • bwa-mem2
  • safestringlib

aarch64-darwin

❌ 1 package failed to build:
  • safestringlib

@GaetanLepage
Copy link
Contributor

Fails on darwin with:

ld: unknown option: -z
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [CMakeFiles/safestring_shared.dir/build.make:226: libsafestring_shared.1.2.0.dylib] Error 1
make[1]: *** [CMakeFiles/Makefile2:147: CMakeFiles/safestring_shared.dir/all] Error 2

@apraga apraga force-pushed the bwa-mem2-fix branch 2 times, most recently from a5cdeb8 to 8e41c2b Compare February 10, 2025 21:39
@apraga
Copy link
Contributor Author

apraga commented Feb 10, 2025

We are in luck : a fix has been proposed upstream. The patch was added : does it compile on darwin now ?

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 380725


x86_64-linux

✅ 2 packages built:
  • bwa-mem2
  • safestringlib

aarch64-linux

✅ 1 package built:
  • safestringlib

x86_64-darwin

❌ 2 packages failed to build:
  • bwa-mem2
  • safestringlib

aarch64-darwin

❌ 1 package failed to build:
  • safestringlib

@GaetanLepage
Copy link
Contributor

GaetanLepage commented Feb 10, 2025

safestrings still fails with:

/tmp/nix-build-safestringlib-1.2.0-unstable-2024-10-21.drv-1/source/unittests/test_memset_s.c:30:35: error: too few arguments to function call, expected 4, have 3
   30 |     rc = memset_s(NULL, LEN, value);
      |          ~~~~~~~~

@apraga
Copy link
Contributor Author

apraga commented Feb 10, 2025

Yes, the tests weren't there initially.... Here's a patch (untested).

@GaetanLepage
Copy link
Contributor

Still failing...

/tmp/nix-build-safestringlib-1.2.0-unstable-2024-10-21.drv-1/source/unittests/test_wcscpy_s.c:289:5: error: call to undeclared function 'wmemset_s'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
  289 |     wmemset_s(str1, L'x', 20);
      |     ^

@apraga
Copy link
Contributor Author

apraga commented Feb 10, 2025

Okay... I've tried to disable this test specifically. Let me know how it goes...

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 380725


x86_64-linux

✅ 2 packages built:
  • bwa-mem2
  • safestringlib

aarch64-linux

✅ 1 package built:
  • safestringlib

x86_64-darwin

❌ 2 packages failed to build:
  • bwa-mem2
  • safestringlib

aarch64-darwin

❌ 1 package failed to build:
  • safestringlib

@apraga
Copy link
Contributor Author

apraga commented Feb 12, 2025

Is it the same error as before ?

@GaetanLepage
Copy link
Contributor

It is now failing at the very end:

----------------END TEST--------------------
Running phase: installPhase
cp: cannot stat '../libsafestring_shared.so': No such file or directory

@apraga
Copy link
Contributor Author

apraga commented Feb 12, 2025

Could you check if libsafestring_shared.so exits on darwin ? If it's just libsafestring_shared.a that is built, or if the name is different, we can manage. If nothing is built, we'll have to mark it as broken I guess. Sorry :/

@GaetanLepage
Copy link
Contributor

Could you check if libsafestring_shared.so exits on darwin ?

Here is what the folder contains at the beginning of the installPhase:

----------------END TEST--------------------
Running phase: installPhase
---------------------------------------
total 356
drwxr-xr-x 13 _nixbld1 nixbld    416 Feb 13 00:00 .
drwxr-xr-x 21 _nixbld1 nixbld    672 Feb 13 00:00 ..
-rw-r--r--  1 _nixbld1 nixbld  25479 Feb 13 00:00 CMakeCache.txt
drwxr-xr-x 16 _nixbld1 nixbld    512 Feb 13 00:00 CMakeFiles
-r--r--r--  1 _nixbld1 nixbld   4346 Feb 13 00:00 CPackConfig.cmake
-r--r--r--  1 _nixbld1 nixbld   4841 Feb 13 00:00 CPackSourceConfig.cmake
-rw-r--r--  1 _nixbld1 nixbld  83647 Feb 13 00:00 Makefile
-rw-r--r--  1 _nixbld1 nixbld   8730 Feb 13 00:00 cmake_install.cmake
-rwxr-xr-x  1 _nixbld1 nixbld  87840 Feb 13 00:00 libsafestring_shared.1.2.0.dylib
lrwxr-xr-x  1 _nixbld1 nixbld     32 Feb 13 00:00 libsafestring_shared.dylib -> libsafestring_shared.1.2.0.dylib
-rw-r--r--  1 _nixbld1 nixbld 125000 Feb 13 00:00 libsafestring_static.a
-rw-r--r--  1 _nixbld1 nixbld   2796 Feb 13 00:00 safestringConfig.cmake
drwxr-xr-x  6 _nixbld1 nixbld    192 Feb 13 00:00 unittests
cp: cannot stat '../libsafestring_shared.so': No such file or directory

To fix compilation issues in bwa-mem2, an update of this library is required.
Latest, unstable version compiles more easily than the last stable release.
@apraga
Copy link
Contributor Author

apraga commented Feb 13, 2025

I've removed the .so alltogether as it's not needed by bwa-mem2. Does that fixes it ?

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 380725


x86_64-linux

✅ 2 packages built:
  • bwa-mem2
  • safestringlib

aarch64-linux

✅ 1 package built:
  • safestringlib

x86_64-darwin

❌ 1 package failed to build:
  • bwa-mem2
✅ 1 package built:
  • safestringlib

aarch64-darwin

✅ 1 package built:
  • safestringlib

@GaetanLepage
Copy link
Contributor

safestringlib now builds fine on all platforms.
bwa-mem2 fails on x86_64-darwin:

clang++ -g -O3 -fpermissive -msse -msse2 -msse3 -mssse3 -msse4.1   src/main.o libbwa.a -lpthread -lm -lz -L. -lbwa -L/nix/store/qxi18xq5pncpb28n46pgg7xx4isc7ff9-safestringlib-1.2.0->
ld: library not found for -l:libsafestring.a
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Makefile:107: bwa-mem2] Error 1

@apraga
Copy link
Contributor Author

apraga commented Feb 13, 2025

Almost there ! Could you check libsafestring.a is indeed in $out/lib for safestring ?

@GaetanLepage
Copy link
Contributor

Almost there ! Could you check libsafestring.a is indeed in $out/lib for safestring ?

nixpkgs on  bwa-mem2-fix
❯ nix-build -A safestringlib --system x86_64-darwin
/nix/store/qxi18xq5pncpb28n46pgg7xx4isc7ff9-safestringlib-1.2.0-unstable-2024-10-21

nixpkgs on  bwa-mem2-fix
❯ ls -l result/lib/
.r--r--r-- 111k root  1 Jan  1970 libsafestring.a

@apraga
Copy link
Contributor Author

apraga commented Feb 14, 2025

I've fixed the linker flag. It should work with clang. I tried to test it by overriding stdenv but it failed on my machine so I defer to you :)

On latest unstable, there are several implicit function declaration.
in safestrlib, which was not a seperate package but simply a submodule.

It is now a separate package (see previous commit) and using latest
unstable for this libary fixes the issue.
@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 380725


x86_64-linux

✅ 2 packages built:
  • bwa-mem2
  • safestringlib

aarch64-linux

✅ 1 package built:
  • safestringlib

x86_64-darwin

✅ 2 packages built:
  • bwa-mem2
  • safestringlib

aarch64-darwin

✅ 1 package built:
  • safestringlib

Copy link
Contributor

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

Good job @apraga !

@GaetanLepage GaetanLepage merged commit 7b75c7a into NixOS:master Feb 14, 2025
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants