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

octoprint: freeze deps and fix build #91417

Merged
merged 4 commits into from Jun 26, 2020
Merged

Conversation

elohmeier
Copy link
Contributor

@elohmeier elohmeier commented Jun 24, 2020

Motivation for this change

Octoprint requires an older version of Werkzeug than in nixpkgs, causing a build failure. This pins the dependency version using the already established approach in the octoprint package and fixes the build.
Also the other build dependencies were fixed.

Things done

Pinned and fixed dependencies.

  • 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.

@gebner
Copy link
Member

gebner commented Jun 24, 2020

@elohmeier Does uvloop build for you? For me it hangs in the test_sock_cancel_add_reader_race test.

@jonringer
Copy link
Contributor

jonringer commented Jun 24, 2020

@elohmeier that's been an issue with that package for months.

I disabled python38 since it was causing me to not be able to review many packages.

3671932

but was effectively reverted in 364909d , although i think it was able to build fine after this ... not sure when it started pausing again

@gebner
Copy link
Member

gebner commented Jun 24, 2020

@jonringer I don't think disabling uvloop on 3.8 is a workable solution. Lots of stuff depends on it and 3.8 is now the default. We should either fix it or disable the tests completely.

@gebner gebner closed this Jun 24, 2020
@gebner gebner reopened this Jun 24, 2020
@gebner
Copy link
Member

gebner commented Jun 24, 2020

Sorry, didn't mean to close the PR. Just clicked on the wrong button.

@jonringer This should fix uvloop: #91440

@jonringer
Copy link
Contributor

@jonringer I don't think disabling uvloop on 3.8 is a workable solution. Lots of stuff depends on it and 3.8 is now the default. We should either fix it or disable the tests completely.

The commit was from January, long before the (python3 -> python38) switch. And I did it so that I could review other packages, I agree it's not a great fix eitiher

@gebner
Copy link
Member

gebner commented Jun 24, 2020

I've merged master. Now it fails in sentry-sdk..

@jonringer
Copy link
Contributor

jonringer commented Jun 24, 2020

please avoid merge commits, it's prefered to do:

git pull -r origin master
git push .. .. --force

over

git pull origin master
git push

@elohmeier elohmeier changed the title octoprint: freeze werkzeug [WIP] octoprint: freeze werkzeug Jun 25, 2020
@elohmeier elohmeier requested a review from FRidh as a code owner June 25, 2020 10:53
@elohmeier
Copy link
Contributor Author

@gebner Thanks for the fix to uvloop, I've rebased onto the latest master containing your changes.
I've also fixed the sentry-sdk and trytond dependencies.

The octoprint build still fails because of conflicting dependency versions, e.g.:

pythonCatchConflictsPhase
Found duplicated packages in closure for dependency 'Werkzeug':
  Werkzeug 0.16.1 (/nix/store/ljzw6hh28lvg5jiclwk697slarrjdlcf-python3.8-Werkzeug-0.16.1/lib/python3.8/site-packages)
  Werkzeug 1.0.1 (/nix/store/2yp5874nib71k55984nsac0hdpbbfh7d-python3.8-Werkzeug-1.0.1/lib/python3.8/site-packages)

I'm not sure how to override them correctly so that they are modified in the transient dependencies. @jonringer do you have an idea how to do that?

@jonringer
Copy link
Contributor

I'm not sure how to override them correctly so that they are modified in the transient dependencies. @jonringer do you have an idea how to do that?

You can look at https://github.com/NixOS/nixpkgs/blob/master/pkgs/tools/admin/awscli/default.nix . This might cause issues, as the package that's bringing in the other werkzeug version likely will needed be patch to accept a different version range.

@elohmeier elohmeier changed the title [WIP] octoprint: freeze werkzeug octoprint: freeze deps and fix build Jun 26, 2020
@elohmeier
Copy link
Contributor Author

elohmeier commented Jun 26, 2020

Got it fixed now. I had to introduce trytond as a python package, so that the packageOverrides can be applied to it.

This PR should be ready to be merged now.

@elohmeier
Copy link
Contributor Author

Thanks for your help @gebner & @jonringer!

@gebner gebner merged commit c041d53 into NixOS:master Jun 26, 2020
@elohmeier elohmeier deleted the octoprint-werkzeug branch June 26, 2020 12:35
@ruuda
Copy link
Contributor

ruuda commented Jul 28, 2020

Adding trytond as a propagated build input causes everything that depends on sentry-sdk to acquire a transitive dependency on simplejson though trytond. The presence of simplejson then causes requests to be have differently.

Is it possible to make the trytond dependency optional? As far as I understand, Sentry has special support for it if you happen to use Tryton, but if you don’t, Sentry should work fine without.

Edit: Just moving it from propagatedBuildInputs to checkInputs works, I’ll open a PR for that.

Edit 2: #94082

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.

None yet

4 participants