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

python3Packages: don't depend on setup hooks' bash eval (part with fewer rebuilds) #354811

Merged
merged 10 commits into from
Nov 13, 2024

Conversation

wolfgangwalther
Copy link
Contributor

This is a part of #352976. Those packages here should cause almost no rebuilds, so targeting master.

@ShamrockLee @emilazy

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.11 Release Notes (or backporting 23.11 and 24.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.

@wolfgangwalther
Copy link
Contributor Author

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review


x86_64-linux

❌ 6 packages failed to build:
  • python311Packages.dtw-python
  • python311Packages.dtw-python.dist
  • python312Packages.dtw-python
  • python312Packages.dtw-python.dist
  • routersploit
  • routersploit.dist
✅ 52 packages built:
  • conan
  • conan.dist
  • fichub-cli
  • fichub-cli.dist
  • home-assistant-component-tests.efergy
  • home-assistant-component-tests.rainforest_raven
  • picard
  • picard.dist
  • python311Packages.aioraven
  • python311Packages.aioraven.dist
  • python311Packages.bottleneck
  • python311Packages.bottleneck.dist
  • python311Packages.certbot-dns-inwx
  • python311Packages.certbot-dns-inwx.dist
  • python311Packages.iso4217
  • python311Packages.iso4217.dist
  • python311Packages.macaddress
  • python311Packages.macaddress.dist
  • python311Packages.orange3
  • python311Packages.orange3.dist
  • python311Packages.pyefergy
  • python311Packages.pyefergy.dist
  • python311Packages.qcodes
  • python311Packages.qcodes-contrib-drivers
  • python311Packages.qcodes-contrib-drivers.dist
  • python311Packages.qcodes.dist
  • python311Packages.recordlinkage
  • python311Packages.recordlinkage.dist
  • python311Packages.vapoursynth
  • python311Packages.vapoursynth.dist
  • python312Packages.aioraven
  • python312Packages.aioraven.dist
  • python312Packages.bottleneck
  • python312Packages.bottleneck.dist
  • python312Packages.certbot-dns-inwx
  • python312Packages.certbot-dns-inwx.dist
  • python312Packages.iso4217
  • python312Packages.iso4217.dist
  • python312Packages.macaddress
  • python312Packages.macaddress.dist
  • python312Packages.orange3
  • python312Packages.orange3.dist
  • python312Packages.pyefergy
  • python312Packages.pyefergy.dist
  • python312Packages.qcodes
  • python312Packages.qcodes-contrib-drivers
  • python312Packages.qcodes-contrib-drivers.dist
  • python312Packages.qcodes.dist
  • python312Packages.recordlinkage
  • python312Packages.recordlinkage.dist
  • python312Packages.vapoursynth
  • python312Packages.vapoursynth.dist

The failing packages are currently failing on master as well.

@ShamrockLee
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 354811


aarch64-linux

❌ 8 packages failed to build:
  • conan
  • conan.dist
  • python311Packages.dtw-python
  • python311Packages.dtw-python.dist
  • python312Packages.dtw-python
  • python312Packages.dtw-python.dist
  • routersploit
  • routersploit.dist
✅ 46 packages built:
  • fichub-cli
  • fichub-cli.dist
  • home-assistant-component-tests.efergy
  • home-assistant-component-tests.rainforest_raven
  • picard
  • picard.dist
  • python311Packages.aioraven
  • python311Packages.aioraven.dist
  • python311Packages.bottleneck
  • python311Packages.bottleneck.dist
  • python311Packages.certbot-dns-inwx
  • python311Packages.certbot-dns-inwx.dist
  • python311Packages.iso4217
  • python311Packages.iso4217.dist
  • python311Packages.macaddress
  • python311Packages.macaddress.dist
  • python311Packages.orange3
  • python311Packages.orange3.dist
  • python311Packages.pyefergy
  • python311Packages.pyefergy.dist
  • python311Packages.qcodes
  • python311Packages.qcodes-contrib-drivers
  • python311Packages.qcodes-contrib-drivers.dist
  • python311Packages.qcodes.dist
  • python311Packages.recordlinkage
  • python311Packages.recordlinkage.dist
  • python312Packages.aioraven
  • python312Packages.aioraven.dist
  • python312Packages.bottleneck
  • python312Packages.bottleneck.dist
  • python312Packages.certbot-dns-inwx
  • python312Packages.certbot-dns-inwx.dist
  • python312Packages.iso4217
  • python312Packages.iso4217.dist
  • python312Packages.macaddress
  • python312Packages.macaddress.dist
  • python312Packages.orange3
  • python312Packages.orange3.dist
  • python312Packages.pyefergy
  • python312Packages.pyefergy.dist
  • python312Packages.qcodes
  • python312Packages.qcodes-contrib-drivers
  • python312Packages.qcodes-contrib-drivers.dist
  • python312Packages.qcodes.dist
  • python312Packages.recordlinkage
  • python312Packages.recordlinkage.dist

@wolfgangwalther
Copy link
Contributor Author

@ShamrockLee Does conan fail on master already or is this a new failure?

@ShamrockLee
Copy link
Contributor

@wolfgangwalther
Copy link
Contributor Author

wolfgangwalther commented Nov 10, 2024

Hm, can't reproduce it on aarch64-darwin, x86_64-darwin or x86_64-linux. pkgsCross.aarch64-multiplatform.conan builds for me - no surprise, because the tests are not run, I think. Don't have a aarch64-linux box available.

github:NixOS/nixpkgs#legacyPackages.aarch64-linux.conan already builds on master, and the build seems to be inside the binary cache. It is a new failure.

Could you try to rebuild this with --check? Just to confirm that the test is not flaky somehow.

Alternatively, could you try to apply just the change to conan's file on master - to make sure we're looking at the right change here and it's not a different change in another file that causes this?

If the removal of -n really causes that... that would be odd.

@ShamrockLee
Copy link
Contributor

I set settings reuse-build-failures --set false on nixbuild.net to rebuild it, but the same test failed again.

I'll also build it on NixOnDroid, but that would take a while because my phone lacks CPU power.

@ofborg build conan

@wolfgangwalther
Copy link
Contributor Author

I set settings reuse-build-failures --set false on nixbuild.net to rebuild it, but the same test failed again.

That was on master? Or is master consistently building even when not taking it from the cache?

@ShamrockLee
Copy link
Contributor

I set settings reuse-build-failures --set false on nixbuild.net to rebuild it, but the same test failed again.

That was on master? Or is master consistently building even when not taking it from the cache?

The failure happens when building github:wolfgangwalther/nixpkgs/no-python-eval-master#legacyPackages.aarch64-linux.conan.

@ShamrockLee
Copy link
Contributor

Oh! Now I get what you mean. I'll build it on master with the --rebuid flag.

@ShamrockLee
Copy link
Contributor

The build turns out to be reproducible on the master branch with the --rebuild flag.

The conan changes here seem trivial. Maybe they trigger the failure just by causing a rebuild.

@ShamrockLee
Copy link
Contributor

The build turns out to be reproducible on the master branch with the --rebuild flag.

The conan changes here seem trivial. Maybe they trigger the failure just by causing a rebuild.

The build log of the master build with --rebuilds by nixbuild.net is similar to that after this change but different from that of OfBorg. Both fail the tests.

@wolfgangwalther
Copy link
Contributor Author

Wow, really odd. I am out of ideas right now 😕

@ShamrockLee
Copy link
Contributor

@HaoZeke, what do you think about Conan's test failure?

@wolfgangwalther
Copy link
Contributor Author

The build log of the master build with --rebuilds by nixbuild.net is similar to that after this change but different from that of OfBorg. Both fail the tests.

Ah, I misread your message earlier. So indeed the test is flaky for aarch64-linux on master already, right?

In this case, I would suggest we just disable the specific test and then continue.

@ShamrockLee
Copy link
Contributor

In this case, I would suggest we just disable the specific test and then continue.

The nixbuild.net aarch64-linux builder fails test/integration/toolchains/google/test_bazeldeps.py::test_shared_windows_find_libraries, and the OfBorg aarch64-linux builder test/integration/remote/auth_test.py::test_token_expired.

To be able to remove the bash-eval behavior for setupPyGlobalFlags, we
change $out to use placeholder "out" instead.

Confirmed that the locales are still loaded from the correct path as
demonstrated in NixOS#284050.
There is only one test and this was disabled via some nasty bash eval
hacks in pytestFlagsArray.

Let's just use... doCheck = false instead?
We'd like to avoid bash eval in pytestFlagsArray, because we want to get
rid of support for it. This case works just fine without.
It makes no sense to depend those folders on pname - they are part of
the source code, not our convention, after all.

Also this avoids using bash eval behavior in pytestFlagsArray.
This surely was intended to be an imports check, judging by the content
of the list. Setting pytestCheckHook like that surely makes no sense.
Those are provided by pytest-xdist's setup hook automatically anyway.
This flag relies on bash eval of pytestFlagsArray, which we'd like to
get rid of. By moving the evaluation of $src into the preCheck hook, we
achieve the same.
We can just cd to $out to let the tests run from that folder
automatically. Additionally we get better test output, because the
/nix/store component is missing from file names.
Avoids the use of bash eval in unittestFlagsArray, which we want to
remove.
This test failed repeatedly on aarch64-linux, so far.
@wolfgangwalther
Copy link
Contributor Author

In this case, I would suggest we just disable the specific test and then continue.

The nixbuild.net aarch64-linux builder fails test/integration/toolchains/google/test_bazeldeps.py::test_shared_windows_find_libraries, and the OfBorg aarch64-linux builder test/integration/remote/auth_test.py::test_token_expired.

I disabled the test_shared_windows_find_libraries test. Let's see whether the test failure of OfBorg is repeatable as well or whether it might have been a load issue or so.

@ShamrockLee Can you test with nixbuild.net again?

@ShamrockLee
Copy link
Contributor

@ofborg build conan

@wolfgangwalther
Copy link
Contributor Author

Conan built on aarch64-linux for OfBorg successfully as well. This should be good to go, imho.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 13, 2024
@emilazy emilazy merged commit 70bad31 into NixOS:master Nov 13, 2024
31 of 33 checks passed
@wolfgangwalther wolfgangwalther deleted the no-python-eval-master branch November 13, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants