-
-
Notifications
You must be signed in to change notification settings - Fork 14.9k
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
nextflow: 22.10.6 -> 24.08.0-edge + remove buildFHSEnv + compile from source + add tests #339197
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
6a1ce54
nextflow: 22.10.6 -> 24.04.4 + remove buildFHSEnv
rollf e7dac85
nextflow: add a passthru.tests.version
rollf 8d15ee1
tests/nextflow: init
rollf 9cd78fd
tests/nextflow: restrict to x86_64-linux
rollf f481bad
nextflow: fix build on darwin
rollf File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
import ./make-test-python.nix ( | ||
{ pkgs, ... }: | ||
let | ||
bash = pkgs.dockerTools.pullImage { | ||
imageName = "quay.io/nextflow/bash"; | ||
imageDigest = "sha256:bea0e244b7c5367b2b0de687e7d28f692013aa18970941c7dd184450125163ac"; | ||
sha256 = "161s9f24njjx87qrwq0c9nmnwvyc6iblcxka7hirw78lm7i9x4w5"; | ||
finalImageName = "quay.io/nextflow/bash"; | ||
}; | ||
|
||
hello = pkgs.stdenv.mkDerivation { | ||
name = "nextflow-hello"; | ||
src = pkgs.fetchFromGitHub { | ||
owner = "nextflow-io"; | ||
repo = "hello"; | ||
rev = "afff16a9b45c8e8a4f5a3743780ac13a541762f8"; | ||
hash = "sha256-c8FirHc+J5Y439g0BdHxRtXVrOAzIrGEKA0m1mp9b/U="; | ||
}; | ||
installPhase = '' | ||
cp -r $src $out | ||
''; | ||
}; | ||
run-nextflow-pipeline = pkgs.writeShellApplication { | ||
name = "run-nextflow-pipeline"; | ||
runtimeInputs = [ pkgs.nextflow ]; | ||
text = '' | ||
export NXF_OFFLINE=true | ||
for b in false true; do | ||
echo "docker.enabled = $b" > nextflow.config | ||
cat nextflow.config | ||
nextflow run -ansi-log false ${hello} | ||
done | ||
''; | ||
}; | ||
in | ||
{ | ||
name = "nextflow"; | ||
|
||
nodes.machine = | ||
{ ... }: | ||
{ | ||
environment.systemPackages = [ | ||
run-nextflow-pipeline | ||
pkgs.nextflow | ||
]; | ||
virtualisation = { | ||
docker.enable = true; | ||
}; | ||
}; | ||
|
||
testScript = | ||
{ nodes, ... }: | ||
'' | ||
start_all() | ||
machine.wait_for_unit("docker.service") | ||
machine.succeed("docker load < ${bash}") | ||
machine.succeed("run-nextflow-pipeline >&2") | ||
''; | ||
} | ||
) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hash is different for aarch64-linux apparently (quoting from ofborg):
I wonder if we should simply do this:
Or disable the test on platforms not
x86_64-linux
. I tend to the do the later, as it would be hard to fix further issues with aarch64-linux we might encounter, and I assume that both of us are losing patience :).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently openjdk doesn't build for me in cross, so I don't think we'll be getting around to running the tests (or the application itself) in cross any time soon anyway. I don't have a big stake in this but I would recommend indeed just disabling the tests for cross.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not talking about cross compilation from x86_64-linux to aarch64-linux, but rather native building of the NixOS test derivation on aarch64-linux.
openjdk
builds fine on aarch64-linux according tohydra-check
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, wow. Why could the hash be different in the first place? I don't have an opinion here so I'd follow @doronbehar suggestions to only support
x86_64-linux
. I would disable the test within thenextflow
-derivation itself (i.e.passthru.tests.default = if host platform ...
), right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make ofborg avoid building that derivation when it looks up
nextflow.passthru.tests
, but still your commitnixos/nextflow
will make at least Hydra try to build that test on aarch64-linux. You can use thehandleTestOn
function that is used inall-tests.nix
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a new commit using
handleTestOn
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the hash is different because the data on the docker image differs among architectures/systems. Maybe one could have both hashes, depending on the
$system
. That might be something for the future.