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.

Sign up for GitHub

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

Jump to bottom

[staging] wafHook: Don't wrap Python 2 #106533

Merged
merged 8 commits into from Dec 19, 2020
Merged

[staging] wafHook: Don't wrap Python 2 #106533

merged 8 commits into from Dec 19, 2020

Conversation

dasJ
Copy link
Member

@dasJ dasJ commented Dec 10, 2020

Motivation for this change

Python 2 is dead, and just replacing it in the hook will most likely break builds that are not yet migrated to Python 3 (looking at you, gaia).

Closes #104701
Related to #69310

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.

@dasJ
Copy link
Member Author

dasJ commented Dec 10, 2020

This is a draft until nixpkgs-review is done building and the broken builds are fixed.

Mic92
Mic92 reviewed Dec 10, 2020
View reviewed changes
pkgs/development/tools/build-managers/wafHook/default.nix

makeSetupHook {
deps = [ python ];
Copy link
Member

Choose a reason for hiding this comment

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

Something to mention in changelogs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, but I'm going to target staging soon and I was told that PRs to staging should not include changelog to prevent annoying conflicts

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I would just mention that there how to fix wafHook usage by also include python in nativeBuildInputs.

@dasJ dasJ force-pushed the refactor/waf-hook branch 2 times, most recently from 98e146b to 9f6d1ac Compare December 14, 2020 08:01
@dasJ dasJ changed the base branch from master to staging December 14, 2020 08:01
@dasJ dasJ marked this pull request as ready for review December 14, 2020 08:01
@dasJ
Copy link
Member Author

dasJ commented Dec 14, 2020

There are still builds failing but this seems unrelated to this PR.
Example of a randomly failing build:

builder for '/nix/store/zh0p4ifn3s3wx48vcyfpvp9kak2ana39-lincity-ng-2.9beta.20170715.drv' failed with exit code 134; last 10 log lines:
  config.status: creating Jamconfig
  config.status: creating config.h
  config.status: executing Jamfile commands
  configure:

  Notice: This project uses jam (and not make) as build tool.

  building
  *** stack smashing detected ***: terminated
  /nix/store/7d2w3lafm8cla7n6ii5jskhsnqg4zv3a-stdenv-linux/setup: line 1310:  1760 Aborted                 (core dumped) AR='ar r' jam -j $NIX_BUILD_CORES

@dasJ dasJ changed the title wafHook: Don't wrap Python 2 [staging] wafHook: Don't wrap Python 2 Dec 14, 2020
FRidh
FRidh reviewed Dec 17, 2020
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Changelog is needed. Other than that, this is the direction I think we should go with wafHook.

This was referenced Dec 17, 2020
Closed
Closed
Closed
@dasJ
Copy link
Member Author

dasJ commented Dec 17, 2020

@FRidh do you want me to add it to this PR to staging?

@FRidh
Copy link
Member

FRidh commented Dec 17, 2020

Yes

@dasJ
Copy link
Member Author

dasJ commented Dec 17, 2020

Done and rebased to current staging

@ajs124 ajs124 added this to Needs review in Python 2 deprecation Dec 19, 2020
@FRidh FRidh merged commit f081225 into NixOS:staging Dec 19, 2020
Python 2 deprecation automation moved this from Needs review to Done Dec 19, 2020
@FRidh
Copy link
Member

FRidh commented Dec 19, 2020

Thank you

@ajs124 ajs124 deleted the refactor/waf-hook branch December 19, 2020 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@FRidh FRidh FRidh left review comments

@jtojnar jtojnar Awaiting requested review from jtojnar

@zowoq zowoq Awaiting requested review from zowoq

@vcunat vcunat Awaiting requested review from vcunat

@ajs124 ajs124 Awaiting requested review from ajs124

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: clean-up 8.has: documentation 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500
Projects
Python 2 deprecation
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dasJ @FRidh @Mic92