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

Jump to bottom

python3Packages.wasmer: make cargoHash invariant to the Python version #110580

Merged
merged 1 commit into from Jan 24, 2021
Merged

python3Packages.wasmer: make cargoHash invariant to the Python version #110580

merged 1 commit into from Jan 24, 2021

Conversation

danieldk
Copy link
Contributor

Motivation for this change

The package name used the Python version. Since the name is used in
the Cargo vendor directory, the Cargo vendor hash depended on the
Python version. Remove the Python version from the name.

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.

@danieldk
python3Packages.wasmer: make cargoHash invariant to the Python version
491667c 
The package name used the Python version. Since the name is used in
the Cargo vendor directory, the Cargo vendor hash depended on the
Python version. Remove the Python version from the name.
@danieldk danieldk mentioned this pull request Jan 23, 2021
Closed
4 tasks
@ofborg ofborg bot requested a review from SuperSandro2000 January 23, 2021 11:47
@SuperSandro2000
Copy link
Member

I tried that initially but the cargoHash changed with different python versions.

@danieldk
Copy link
Contributor Author

danieldk commented Jan 23, 2021 •
edited

I tried that initially but the cargoHash changed with different python versions.

Could you elaborate what you mean? With this change, you can build wasmer with different Python versions without changing the hash.

@danieldk
Copy link
Contributor Author

@ofborg build python37Packages.wasmer python38Packages.wasmer python39Packages.wasmer

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 110580 run on x86_64-linux 1

1 package marked as broken and skipped:
  • python39Packages.starlette
2 packages failed to build and are new build failures:
14 packages built:
  • python37Packages.fastapi
  • python37Packages.fastdiff
  • python37Packages.graphene
  • python37Packages.snapshottest
  • python37Packages.starlette
  • python37Packages.wasmer
  • python38Packages.fastdiff
  • python38Packages.graphene
  • python38Packages.snapshottest
  • python38Packages.wasmer
  • python39Packages.fastdiff
  • python39Packages.graphene
  • python39Packages.snapshottest
  • python39Packages.wasmer

@SuperSandro2000
Copy link
Member

Could you elaborate what you mean? With this change, you can build wasmer with different Python versions without changing the hash.

When I put in a wrong hash and tried building it with python 3.7, 3.8 and 3.9 I would get a different hash each time even before I used the python version in the dep.s

@danieldk
Copy link
Contributor Author

danieldk commented Jan 23, 2021 •
edited

When I put in a wrong hash and tried building it with python 3.7, 3.8 and 3.9 I would get a different hash each time even before I used the python version in the dep.s

Putting in fakeHash (with this PR):

$ nix-build -A python37Packages.wasmer
  specified: sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
     got:    sha256-Rq5m9Lu6kePvohfhODLMOpGPFtCh0woTsQY2TufoiNQ=
$ nix-build -A python38Packages.wasmer
  specified: sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
     got:    sha256-Rq5m9Lu6kePvohfhODLMOpGPFtCh0woTsQY2TufoiNQ=
$ nix-build -A python39Packages.wasmer
  specified: sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
     got:    sha256-Rq5m9Lu6kePvohfhODLMOpGPFtCh0woTsQY2TufoiNQ=

The Python version should have no influence on vendoring. If it had, it would be a bug in our fetchCargoTarball.

@SuperSandro2000
Copy link
Member

Thanks for fixing this. I am not sure actually why I did not do it in the first place like this.

@esotericnonsense
Copy link
Contributor

thank you; this is a far better solution than my original PR. 👌

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Jan 24, 2021 •
edited

This is a semi-automatic executed nixpkgs-review which is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 110580 run on x86_64-linux 1

1 package marked as broken and skipped:
  • python39Packages.starlette
2 packages failed to build and are new build failures:
14 packages built:
  • python37Packages.fastapi
  • python37Packages.fastdiff
  • python37Packages.graphene
  • python37Packages.snapshottest
  • python37Packages.starlette
  • python37Packages.wasmer
  • python38Packages.fastdiff
  • python38Packages.graphene
  • python38Packages.snapshottest
  • python38Packages.wasmer
  • python39Packages.fastdiff
  • python39Packages.graphene
  • python39Packages.snapshottest
  • python39Packages.wasmer

Not sure why this fails but since we are building a new package I am going to ignore it for now.

@SuperSandro2000 SuperSandro2000 merged commit a3dcc62 into NixOS:master Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@danieldk @SuperSandro2000 @esotericnonsense