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

yubioath-flutter: 6.1.0 -> 6.2.0 #230674

Merged
merged 6 commits into from
Aug 13, 2023
Merged

Conversation

afh
Copy link
Member

@afh afh commented May 8, 2023

Description of changes

This is a follow-up on #228372 with a kind request to @Lassulus to test the changes on my behalf on Linux.

❓ Is it possible to make the substituteInPlace more generic by using a regex, e.g.: --replace 'yubikey-manager = ".*"'…

ℹ️ I'm not sure whether the vendorHash would need / benefit from an update too.

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.05 Release Notes (or backporting 22.11 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.

@Lassulus
Copy link
Member

Lassulus commented May 8, 2023

error: builder for '/nix/store/j92zv22vpqglrb2swsknyqayrsvx4w0q-yubioath-flutter-helper-6.2.0.drv' failed with exit code 1;
       last 10 log lines:
       > Finished creating a wheel...
       > Finished executing pipBuildPhase
       > installing
       > Executing pipInstallPhase
       > /build/source/helper/dist /build/source/helper
       > Processing ./authenticator_helper-0.1.0-py3-none-any.whl
       > Requirement already satisfied: zxing-cpp in /nix/store/lj52z8f3cmlll6bl6sgrmwvi7kcn0a3a-python3.10-zxing_cpp-1.4.0/lib/python3.10/site-packages (from authenticator-helper==0.1.0) (1.4.0)
       > ERROR: Could not find a version that satisfies the requirement yubikey-manager==5.1.0 (from authenticator-helper) (from versions: none)
       > ERROR: No matching distribution found for yubikey-manager==5.1.0
       >
       For full logs, run 'nix log /nix/store/j92zv22vpqglrb2swsknyqayrsvx4w0q-yubioath-flutter-helper-6.2.0.drv'

@Aleksanaa
Copy link
Member

@afh
Copy link
Member Author

afh commented May 8, 2023

Probably yes, good find @Aleksanaa. As I'm out and about it'll take me until tomorrow before I'm able to update the PR.

@Kranzes
Copy link
Member

Kranzes commented May 8, 2023

Can't you relax that's dependency? With pythonRelaxDeps or whatever that's called

@afh afh force-pushed the update-yubioath-flutter branch from bf98976 to 1baf7a0 Compare May 9, 2023 09:29
@afh
Copy link
Member Author

afh commented May 9, 2023

Thanks for the suggestion, @Kranzes. I didn't know such thing existed in nixpkgs (TIL!). I've made related changes to try that out.

@Lassulus
Copy link
Member

Lassulus commented May 9, 2023

hmm, no changes in build errors but eval now takes 10x longer

rror: builder for '/nix/store/bj1fnmxkbw03fbcwdhqj0wlyp922023c-yubioath-flutter-helper-6.2.0.drv' failed with exit code 1;
       last 10 log lines:
       > /build/source/helper
       > Finished executing pipBuildPhase
       > installing
       > Executing pipInstallPhase
       > /build/source/helper/dist /build/source/helper
       > Processing ./authenticator_helper-0.1.0-py3-none-any.whl
       > Requirement already satisfied: zxing-cpp in /nix/store/lj52z8f3cmlll6bl6sgrmwvi7kcn0a3a-python3.10-zxing_cpp-1.4.0/lib/python3.10/site-packages (from authenticator-helper==0.1.0) (1.4.0)
       > ERROR: Could not find a version that satisfies the requirement yubikey-manager==5.1.0 (from authenticator-helper) (from versions: none)
       > ERROR: No matching distribution found for yubikey-manager==5.1.0
       >
       For full logs, run 'nix log /nix/store/bj1fnmxkbw03fbcwdhqj0wlyp922023c-yubioath-flutter-helper-6.2.0.drv'.

@lukegb
Copy link
Contributor

lukegb commented May 11, 2023

I think for pythonRelaxDeps to work, you'll need to add the python3.pkgs.pythonRelaxDepsHook to nativeBuildInputs.

@afh afh force-pushed the update-yubioath-flutter branch from 1baf7a0 to 248d72b Compare May 11, 2023 10:59
@afh
Copy link
Member Author

afh commented May 11, 2023

Thanks for the suggestions @lukegb. Where can I learn more about the difference between python3Packages and python3.pkgs?

@afh afh force-pushed the update-yubioath-flutter branch from 248d72b to 36fb23c Compare May 11, 2023 12:41
@hacker1024 hacker1024 mentioned this pull request May 11, 2023
12 tasks
@afh afh force-pushed the update-yubioath-flutter branch 2 times, most recently from 9b8821c to ddd20cb Compare May 11, 2023 19:19
@lukegb
Copy link
Contributor

lukegb commented May 12, 2023

Thanks for the suggestions @lukegb. Where can I learn more about the difference between python3Packages and python3.pkgs?

Ah, sorry, I missed that this was in helper.nix. helper.nix is called as python3.pkgs.callPackage, so it's already in the context of the Python package set, so you can (and should) refer to that as just pythonRelaxDepsHook.

python3Packages and python3.pkgs are usually the same - it can sometimes be more convenient to access the package set through python3.pkgs if you already have python3 in context IMO. Not sure what the recommended best practice is, though.

@afh
Copy link
Member Author

afh commented May 12, 2023

Thanks for the explanation, @lukegb, it verifies my assumption that pythonPackages and python3.pgks can be used (more or less) interchangeably.

I did try and use only pythonRelaxDepsHook without the python3.pkgs prefix, yet that failed the tests.

Unfortunately something seems to be missing since the test run for the latest changes with python3.pkgs. pythonRelaxDepsHook in nativeBuildDependencies fails with: No matching distribution found for yubikey-manager==5.1.0.

Any other ideas to approach this?

@lukegb
Copy link
Contributor

lukegb commented May 12, 2023

Ah. pythonRelaxDepsHook doesn't seem to work for pyproject builds.

My advice then is to extend the existing postPatch which relaxes dependencies - just adding two new lines following the existing format for Pillow (we have 9.4 at the moment, 9.5 is coming in #228493), and for yubikey-manager should be enough.

@Kranzes
Copy link
Member

Kranzes commented May 12, 2023

Ah. pythonRelaxDepsHook doesn't seem to work for pyproject builds.

My advice then is to extend the existing postPatch which relaxes dependencies - just adding two new lines following the existing format for Pillow (we have 9.4 at the moment, 9.5 is coming in #228493), and for yubikey-manager should be enough.

I am clearly seeing it being used across nixpkgs with pyproject.

@lukegb
Copy link
Contributor

lukegb commented May 12, 2023

Ah. pythonRelaxDepsHook doesn't seem to work for pyproject builds.
My advice then is to extend the existing postPatch which relaxes dependencies - just adding two new lines following the existing format for Pillow (we have 9.4 at the moment, 9.5 is coming in #228493), and for yubikey-manager should be enough.

I am clearly seeing it being used across nixpkgs with pyproject.

Oh, you're right. Looking more closely at the implementation, it's because our pname and version don't match our wheel output - we'd have to set pname to authenticator-helper and version to 0.1.0 to make it work, which we could do, but that generates a completely useless output path, and it's probably "nicer" to have it tied more closely to the actual yubioath name/version, rather than having a generic name. But maybe that's outweighed by the utility of just using pythonRelaxDeps instead of substituteInPlace.

@afh
Copy link
Member Author

afh commented May 12, 2023

Thanks everyone for chiming in, very help- and insightful! As a non-native speaker I'm having a bit of trouble "deciphering" the preferred approach from the comments above and kindly request a vote on the issue:

🚀 Change pname to authenticator-helper and version to 0.1.0 and accept a useless output path is generated

👀 add substituteInPlace to change the yubikey-manager in pyproject.toml the postPatch phase.

A question about approach 👀 I have is: Would a change in pyproject.toml be sufficient or would poetry.lock also need to be changed? If yes, what's the best way to deal with the referenced hashes?

@lobre
Copy link
Contributor

lobre commented Aug 10, 2023

It seems yubioath-flutter still does not build as of today. Would anyone be able to report on the status of this PR? How far is it from being merged? Thanks.

@lukegb
Copy link
Contributor

lukegb commented Aug 10, 2023

It seems yubioath-flutter still does not build as of today. Would anyone be able to report on the status of this PR? How far is it from being merged? Thanks.

It should still build, and is indeed successfully being built by Hydra. Can you please explain where you're seeing build failures and what system type you're on?

[lukegb@totoro:~/Projects/nixpkgs/check-yubioath-builds]$ git rev-parse HEAD
b8392f9eb12070b647d711c3964ebc89c5d34854

[lukegb@totoro:~/Projects/nixpkgs/check-yubioath-builds]$ nix-build -A yubioath-flutter
/nix/store/q162ijbka1rbkxkh9ljvzfhnmwc1mpdv-yubioath-flutter-6.1.0

[lukegb@totoro:~/Projects/nixpkgs/check-yubioath-builds]$ $(nix-build -A nix-info)/bin/nix-info -m
these paths will be fetched (0.28 MiB download, 1.33 MiB unpacked):
  /nix/store/94x80fp2gyj14l6w4b8yn7kkgshifh2a-nix-info
  /nix/store/jids6qqs36p4ds9ghpkcw57kbhp8lbq2-findutils-4.9.0
copying path '/nix/store/jids6qqs36p4ds9ghpkcw57kbhp8lbq2-findutils-4.9.0' from 'https://cache.nixos.org'...
copying path '/nix/store/94x80fp2gyj14l6w4b8yn7kkgshifh2a-nix-info' from 'https://cache.nixos.org'...
 - system: `"x86_64-linux"`
 - host os: `Linux 5.15.61, NixOS, 23.11 (Tapir), 23.11pre-git`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.3.16`
 - channels(root): `"nixos-22.05pre375061.c777cdf5c56"`
 - channels(lukegb): `""`
 - nixpkgs: `/home/lukegb/depot/third_party/nixpkgs`

@lobre
Copy link
Contributor

lobre commented Aug 11, 2023

Can you please explain where you're seeing build failures and what system type you're on?

It seems that I can build it correctly now. False positive then, I am not sure what happened last time I tried. Sorry about that.

@afh
Copy link
Member Author

afh commented Aug 11, 2023

Thanks @lukegb and @lobre for checking in on this PR. From your point of view what is necessary in order to move forward here?

@lukegb
Copy link
Contributor

lukegb commented Aug 13, 2023

I think it's just the 👀 suggestion, that's mostly what I've been waiting on. I've got some local commits that'll fix this up so I'll push them up to your branch and rebase closer to head.

…0a6fd5db59314

Using fetchurl like this means that, if you build the `flutter`
derivation first, you will get a file named "LICENSE" in your store with
the correct hash. `flutter37` will then build because this file is
already in your store, even though the LICENSE to which _it_ refers is
different. This is dangerous in this case - but an intentional design
decision in the way fetchurl works to allow artifacts which are the same
to be fetched from arbitrary sources, or even pre-populated into the
store.

To avoid this, explicitly tag the fetchurl with a name and the commit
hash we're fetching from. This means we _must_ fetch these separately
for each flutter version and avoids the problem of accidentally reusing
artifacts for a different build.
@lukegb lukegb merged commit 5601964 into NixOS:master Aug 13, 2023
22 checks passed
@lukegb
Copy link
Contributor

lukegb commented Aug 13, 2023

Thanks again for your contribution and sorry it's taken such a long time to get it landed.

@afh afh deleted the update-yubioath-flutter branch August 13, 2023 15:44
@afh
Copy link
Member Author

afh commented Aug 13, 2023

Thanks for merging, @lukegb. All good, sometimes things take a bit more time and that's perfectly alright, better than moving (too) fast and breaking things :)

mkg20001 pushed a commit to mkg20001/nixpkgs that referenced this pull request Sep 25, 2023
yubioath-flutter: 6.1.0 -> 6.2.0
(cherry picked from commit 5601964)
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.

6 participants