graalvm-oracle: init at 22#321026
Conversation
pkgs/top-level/all-packages.nix
Outdated
There was a problem hiding this comment.
This used to be called GraalVM EE, now just "Oracle GraalVM". Maybe oracle-graalvm would be a better name here?
|
Do you plan on adding the other supported versions by oracle's graalvm? Such as for JDK 17 and 21. |
|
@rafaelrc7 I don't have any use for them but I'll take a look |
|
Rebased on latest nixpkgs to resolve conflicts |
Thanks! I am currently working on packaging pkl in #286658 and the native build depends on graalvm 17, so it would be a lot of help. |
pkgs/top-level/all-packages.nix
Outdated
There was a problem hiding this comment.
I don't like that we have 2 different graalvmPackages.
I think since we are introducing Oracle GraamVM again, it is a good time to move everything to graalvmPackages and make graalvmCEPackages an alias to graalvmPackages, and deprecate and eventually remove graalvmCEPackages.
There was a problem hiding this comment.
I can do that, but can you be more specific? Do we want graalvmPackages to temporarily be the CE packages and also graalvmPackages.oracle + graalvmPackages.ce as the long-term, stable names?
Do we also want this nesting for JDK packages or is pkgs.oracle-graalvm fine as it is in the current version of the PR?
There was a problem hiding this comment.
I can do that, but can you be more specific? Do we want graalvmPackages to temporarily be the CE packages and also graalvmPackages.oracle + graalvmPackages.ce as the long-term, stable names?
Yes. But probably the names will be graalvmPackages.graalvm-oracle and graalvmPackages.graalvm-ce, because there are other packages inside graalvmCEPackages too.
There was a problem hiding this comment.
I've updated the PR, please let me know what you think
There was a problem hiding this comment.
Not sure if this URL is deterministic, there is no /archive/ version for these
There was a problem hiding this comment.
I think these hashes are correct, I calculated them with nix-prefetch-url but I need to double check.
|
Thanks for the feedback @chayleaf , updated |
pkgs/top-level/aliases.nix
Outdated
There was a problem hiding this comment.
| graalvmCEPackages = graalvmPackages; | |
| graalvm-ce = graalvmPackages.graalvm-ce; | |
| graalvmCEPackages = graalvmPackages; # 2024-08-10 |
aliases.nix is for deprecated aliases, so you have to put the date in there as a rough estimate of whether it's okay to replace the alias with a throw (see maintainers/scripts/remove-old-aliases.py)
graalvm-ce is probably fine to not deprecate? in that case, just put it back to all-packages.nix
There was a problem hiding this comment.
graalvm-ce now lives in graalvmPackages.graalvm-ce so I'm deprecating this old name #321026 (comment)
There was a problem hiding this comment.
Those URLe are unacceptable, especially in this case since Hydra will not cache the result (because this is unfree).
We need either to have a stable URL from upstream or have the files archived somewhere (e.g.: archive.org).
There was a problem hiding this comment.
there are some precedents, but they typically result in the packages being marked as broken (see fmod)
There was a problem hiding this comment.
I found more stable links (with full version strings) here.
I'm not sure if this is sufficient from what you're saying. This package will always be unfree, regardless where we download it from? And I'm not sure it's legal to replicate it to archive.org.
There was a problem hiding this comment.
the ones with full version strings should be sufficient (there are some for 17 as well)
There was a problem hiding this comment.
You are adding 2 separate versions but the commit only says graalvm-oracle: init at 22.
There was a problem hiding this comment.
I've split it up into two commits. I'll do some updates and introduce version 23 in a follow up PR
e3102e9 to
954e7b5
Compare
|
|
So babashka fails on this test: https://github.com/NixOS/nixpkgs/blob/394571358ce82dff7411395829aa6a3aad45b907/pkgs/development/interpreters/babashka/default.nix#L38C1-L39C1
It outputs I tried it on my other NixOS machine and it works: I don't know how to debug this deeper than this, but I think something from the environment is leaking |
954e7b5 to
c85e417
Compare
There was a problem hiding this comment.
This file still seem to be missing the latest changes from master.
There was a problem hiding this comment.
This is the parent of my commits 3b74ae1
I don't think I'm missing anything
There was a problem hiding this comment.
Just compare this file with the one from master: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/compilers/graalvm/community-edition/buildGraalvm.nix.
It is pretty clear that something went wrong. Probably because you moved the file before the changes.
There was a problem hiding this comment.
Oh I know the reason, your PR introduces another file called buildGraalvm in another path and forgot to delete the previous one (the one inside community-edition). So it didn't get any conflicts (because for git there is no conflicts, just a new file) and now we are building from the wrong file.
There was a problem hiding this comment.
Or in other words, this should appear in the diff as a move, not as a new file.
|
|
@ofborg build jet babashka clj-kondo I don't have a darwin machine to test locally |
|
There's something non-deterministic about it. I'm able to build babashka on my branch, on the machine that got the encoding error before. |
This non-deterministic behavior was fixed a long time ago in |
c85e417 to
fb911fa
Compare
|
Good catch, thanks. That file is gone now and there's only the one |
|
thiagokokada
left a comment
There was a problem hiding this comment.
Code-wise, LGTM.
Will run nixpkgs-review for aarch64-darwin and x86_64-darwin, and also wait until the eval checks pass to merge.
|
|
What a journey! Thanks. I don't know what's going with ofborg, I haven't seen it be green anytime recently |
|
It is slower, but still running. It should take 1 or 2 days to eval your PR, but it should happen eventually. |
|
@thiagokokada it's green, can you merge? |
Supersedes #259639, resolves #259631
Description of changes
This adds oracle-graalvm and make buildGraalvm more generic to be used on other places than community-edition.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Pinging @thiagokokada (reviewer from the previous PR)
Add a 👍 reaction to pull requests you find important.