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.flitBuildHook: remove #254309

Merged
merged 68 commits into from
Sep 25, 2023
Merged

python3Packages.flitBuildHook: remove #254309

merged 68 commits into from
Sep 25, 2023

Conversation

pbsds
Copy link
Member

@pbsds pbsds commented Sep 9, 2023

Description of changes

This change removes all instances of format = "flit";, and removes flitBuildHook.
This backward-seeming order is to be git-bisect friendly.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@github-actions github-actions bot added 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Sep 9, 2023
@dotlambda dotlambda changed the title buildFlitHook: remove python3Packages.flitBuildHook: remove Sep 9, 2023
@pbsds
Copy link
Member Author

pbsds commented Sep 9, 2023

This will likely break many of the overrides defined in poetry2nix

@pbsds pbsds marked this pull request as ready for review September 9, 2023 23:40
@pbsds pbsds requested a review from adisbladis as a code owner September 9, 2023 23:54
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Note the typo in the commit message.

@pbsds
Copy link
Member Author

pbsds commented Sep 10, 2023

I think I need some pointers on the eval error. I'm not even sure how to reproduce it locally.

@figsoda
Copy link
Member

figsoda commented Sep 10, 2023

The eval error looks like nixops_unstable, which uses poetry2nix

@@ -26,6 +27,10 @@ buildPythonPackage rec {
hash = "sha256-XVPYr7JwxeZfZ68+vQ7a7MNiAfJ2bvMbM3R1ryVJ+OU=";
};

nativeBuildInputs = [
flit
Copy link
Member

Choose a reason for hiding this comment

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

How come it uses flit instead of flit-core?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Preferred solution here is to patch the pyproject.toml.

@@ -290,3 +290,5 @@ The module update takes care of the new config syntax and the data itself (user
./common/auto-format-root-device.nix ];` When you use the systemd initrd, you
can automatically format the root device by setting
`virtualisation.fileSystems."/".autoFormat = true;`.

- `python3.pkgs.flitBuildHook` has been removed. Use `flit-core` and `format = "pyproject"` instead.
Copy link
Member

Choose a reason for hiding this comment

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

Please see the comment underneath the headline

Copy link
Member Author

Choose a reason for hiding this comment

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

Please elaborate

Copy link
Member

Choose a reason for hiding this comment

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

changelog sections should be randomly sorted and not appeneded

@@ -21,6 +22,19 @@ buildPythonPackage rec {
sha256 = "ExZy5k5RE7k+D0lGmuIkGWrWQ+m24K2oqbUEg4BAVuY=";
};

# patch below won't apply due to different line endings
prePatch = ''
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
prePatch = ''
postPatch = ''

Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to run before patchPhase where patches are applied

@pbsds
Copy link
Member Author

pbsds commented Sep 24, 2023

Full disclosure, I just discovered #251880, a duplicate effort. I thought it was odd how every time i needed a patch, @tjni seemed to have just submitted one to the upstream. I tried searching for whether he had a nixpkgs pull, but for some reason it did not show up with my search terms. His work also targets additional issues such as darwin compat, while this PR stricly removes flitBuildHook only.

@FRidh FRidh requested a review from dotlambda September 25, 2023 09:33
@FRidh FRidh merged commit f4822bb into NixOS:master Sep 25, 2023
11 checks passed
@FRidh
Copy link
Member

FRidh commented Sep 25, 2023

Unmerged changes in #251880 can be rebased later.

@pbsds
Copy link
Member Author

pbsds commented Sep 25, 2023

@FRidh This pull may have triggered a build failiure in tifffile, addressed in #257267.

EDIT: false alert, the hydra job was cancelled, but the logs indicate a successfull build

getchoo added a commit to getchoo-archive/guzzle_api that referenced this pull request Sep 28, 2023
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.

5 participants