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

julia: init julia_10-bin and julia_16-bin #123188

Merged
merged 2 commits into from May 17, 2021
Merged

julia: init julia_10-bin and julia_16-bin #123188

merged 2 commits into from May 17, 2021

Conversation

ghost
Copy link

@ghost ghost commented May 16, 2021

Motivation for this change

Recent attempts to package Julia v1.6.x and unbreak v1.0.x have failed and stalled. As it stands, we know that we have test failures and breakages for the source-based expressions in Nixpkgs. As a response to this, I adapted an older expression that uses autoPatchelfHook on the official binary releases and added a few patches to resolve test failures. This is what you are seeing in this PR.

Note that I am currently only supporting x86_64-linux, but expect me to add i686-linux and aarch64-linux as soon as I have access to hardware that will allow me to develop and test them. My strategy is to get on the NixOS aarch64 build box, but pointers for sensible development and testing for i686-linux would be welcome. If the merging of this PR drags on I will try to add support for them here, otherwise expect swift PRs to follow.

I think there are good reasons to maintain both source and binary packages for Julia given the complexity that comes with packaging it – as seen in the other issue and failures to get v1.6.x into a passable state. Furthermore, I have managed to get system image patching working, so that we could even potentially have a binary release with patches necessary to unbreak the parts of the Julia package ecosystem that is currently failing for both source and binary releases on Nix. Note that this is not in this PR as it is not necessary to get a binary release on par with our existing source-based packages.

Minor note, patches lifted from my Julia fork for the relevant releases. Thankfully these could be recycled from my failed attempts at source builds.

@7c6f434c, @garrison, and @rbvermaa: I am currently standing as sole maintainer for this one, but if you are up for it I am happy to add you to share the “load” as you stand as maintainers for the source-based Julia packages.

@samuela: I am still unable to develop and test on x86_64-darwin, so if you (or anyone else) want to (well “can”, really) maintain it I would be very thankful.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Edit: Spelling and grammar.

cp -r . $out
# Setting `LD_LIBRARY_PATH` resolves `Libdl` failures. Not sure why this is
# only necessary on v1.0.x and a cleaner solution is welcome, but after
# staring at `strace` for a few hours this is as clean as I could make it.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe 1.0 adds $out/lib to RPATH but links no libraries (using dlopen instead), so Nixpkgs automatic patchelf removes it from RPATH?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, that could be it. Will investigate.

Copy link
Author

Choose a reason for hiding this comment

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

Yep, I suspect you are bang on:

> objdump -x lib/libjulia.so | grep RPATH
  RPATH                $ORIGIN/julia:$ORIGIN
> objdump -x /nix/store/pxs6qkddkhpvk1hpkahgz4w18ahfd3m1-julia-bin-1.0.5/lib/libjulia.so | grep RPATH
>

Digging through the manual I thought that what I should do was to set dontPatchELF to true, but I still end up with the RPATH being removed.

Copy link
Author

Choose a reason for hiding this comment

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

If this is not easily addressable, I am happy to merge this as is and try to clean it up later.

homepage = "https://julialang.org";
# Bundled and linked with various GPL code, although Julia itself is MIT.
license = lib.licenses.gpl2Plus;
maintainers = with lib.maintainers; [ ninjin ];
Copy link
Member

Choose a reason for hiding this comment

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

Fine with being added. For example, I will be notified when it's time to merge updates…

Copy link
Author

Choose a reason for hiding this comment

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

Happy to have you onboard! Added and pushed.

@7c6f434c
Copy link
Member

Looks good in general

description = "High-level, high-performance dynamic language for technical computing";
homepage = "https://julialang.org";
# Bundled and linked with various GPL code, although Julia itself is MIT.
license = lib.licenses.gpl2Plus;
Copy link
Author

Choose a reason for hiding this comment

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

Unrelated to this PR, but does this mean that we should chance the license from MIT to GPL 2+ for our source-based Julia packages as well?

Copy link
Member

Choose a reason for hiding this comment

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

Nobody is sure what exactly our license metadata means in any non-trivial case, so …

Here it is clear: here a GPL2+ artifacy is downloaded.

Pontus Stenetorp added 2 commits May 16, 2021 14:12
Relevant patches lifted from the `release-1.6` branch of my Julia fork:

    https://git.sr.ht/~ninjin/julia-nix/tree/release-1.6
Almost as clean as it could be, but was forced to set `LD_LIBRARY_PATH`
to work around a `Libdl` failure that is unique to v1.0.x.
@ofborg ofborg bot requested a review from 7c6f434c May 16, 2021 14:22
Copy link
Member

@samuela samuela left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just a little confused about the comments.

Comment on lines +14 to +16
# Julia’s source files are in different locations for source and binary
# releases. Thus we temporarily create symlinks to allow us to share patches
# with source releases.
Copy link
Member

Choose a reason for hiding this comment

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

I don't really understand this comment. AFAICT there's no overlap between the patches used for the source and binary derivations. All of the patches seem to be split up: patches/1.0, patches/1.0-bin, etc.

Copy link
Author

Choose a reason for hiding this comment

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

It is not referring to the location in the Nixpkgs source tree, but rather the paths in the patches. If we do not do this trick we would have to alter the patches themselves as the paths are different between the Julia release tarballs.

Comment on lines +14 to +16
# Julia’s source files are in different locations for source and binary
# releases. Thus we temporarily create a symlink to allow us to share patches
# with source releases.
Copy link
Member

Choose a reason for hiding this comment

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

Same as my other comment.

Copy link
Author

Choose a reason for hiding this comment

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

Likewise.

@@ -11113,6 +11113,13 @@ in
julia-stable = julia_15;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should reshuffle some of these as well. In particular, I don't think it makes sense for any of the "flagship" attributes to point to julia_15 since that one's broken.

Also I think an alias julia_16 = julia_16-bin would make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 julia_16 = julia_16-bin

Copy link
Author

Choose a reason for hiding this comment

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

I agree that this makes sense, but I would rather address that in a separate PR and/or issue.

@samuela
Copy link
Member

samuela commented May 17, 2021

I took a quick stab at getting this running on x86_64-darwin, but the test suite isn't happy: https://gist.github.com/samuela/257b66127bceffcc11453a6992a92d3d. It's something having to do with spawning the test threads, potentially related to the net_on adventures...

@samuela
Copy link
Member

samuela commented May 17, 2021

But getting it building on x86_64-darwin shouldn't be a requirement for this PR IMHO. We can always follow up later to broaden platform support.

@7c6f434c 7c6f434c merged commit 183350a into NixOS:master May 17, 2021
@ghost ghost deleted the julia-bin branch May 17, 2021 12:25
samuela added a commit to samuela/nixpkgs that referenced this pull request May 17, 2021
Follow up to NixOS#123188. Now that we have binary builds for julia v1.6.x, we should alias `julia_16` to point to `julia_16-bin`.
@samuela samuela mentioned this pull request May 17, 2021
10 tasks
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