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

aws-sam-cli: 0.14.2 -> 0.16.1 #62011

Merged
merged 3 commits into from May 25, 2019
Merged

aws-sam-cli: 0.14.2 -> 0.16.1 #62011

merged 3 commits into from May 25, 2019

Conversation

dhl
Copy link
Contributor

@dhl dhl commented May 24, 2019

Motivation for this change

To bring aws-sam-cli up-to-date and fix pythonPackages.serverlessrepo, which was made broken by pyyaml being updated to a newer version ni nixpkgs.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@dhl dhl requested a review from FRidh as a code owner May 24, 2019 15:40
@Ma27
Copy link
Member

Ma27 commented May 24, 2019

Building aws-sam-cli from your branch fails with the following error on my machine:

Collecting aws-sam-translator==1.10.0 (from aws-sam-cli==0.16.1)
  Could not find a version that satisfies the requirement aws-sam-translator==1.10.0 (from aws-sam-cli==0.16.1) (from versions: )
No matching distribution found for aws-sam-translator==1.10.0 (from aws-sam-cli==0.16.1)
builder for '/nix/store/hi4v4y7a2sl24fsf824c9xicazdq9b2h-aws-sam-cli-0.16.1.drv' failed with exit code 1
error: build of '/nix/store/hi4v4y7a2sl24fsf824c9xicazdq9b2h-aws-sam-cli-0.16.1.drv' on 'ssh://builder' failed: builder for '/nix/store/hi4v4y7a2sl24fsf824c9xicazdq9b2h-aws-sam-cli-0.16.1.drv' failed with exit code 1

@dhl dhl force-pushed the dhl/aws-sam-and-lambda-builders branch from 81023d5 to 47e31ab Compare May 25, 2019 01:40
@dhl
Copy link
Contributor Author

dhl commented May 25, 2019

Thanks for reviewing this, @Ma27!

I forgot to rebuild the aws-sam-cli after rebase, and aws-sam-translator was also updated.

I've updated aws-sam-cli to pin aws-sam-translator at 0.10.0

@Mic92
Copy link
Member

Mic92 commented May 25, 2019

@GrahamcOfBorg build aws-sam-cli

@Ma27
Copy link
Member

Ma27 commented May 25, 2019

Built and tested locally, nix-review seems fine 👍

@Ma27 Ma27 merged commit 35961a1 into NixOS:master May 25, 2019
@Ma27
Copy link
Member

Ma27 commented May 25, 2019

@dhl thanks!

@dhl
Copy link
Contributor Author

dhl commented May 25, 2019

Thank you @Ma27!

@dhl dhl deleted the dhl/aws-sam-and-lambda-builders branch May 25, 2019 11:16
FRidh
FRidh reviewed May 25, 2019
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.

This needs to be reverted.

pkgs/development/python-modules/serverlessrepo/default.nix
let
py = python.override {
packageOverrides = self: super: {
pyyaml = super.pyyaml.overridePythonAttrs (oldAttrs: rec {
Copy link
Member

Choose a reason for hiding this comment

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

overriding a Python library inside python-packages.nix is going to cause trouble when composing an env consisting of the original version and the overridden version

Copy link
Contributor Author

@dhl dhl May 25, 2019 •
edited

Choose a reason for hiding this comment

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

@FRidh understood. I'll get a fix in ASAP.

serverlessrepo has a dependency on pyyaml declared as 'pyyaml~=3.12'.

What's the best way to deal with this? Should I fix up the version in setup.py, or package an older version of pyyaml?

Copy link
Member

Choose a reason for hiding this comment

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

a) check (yourself/with upstream) whether it actually does work with the version we use, and if so patch it
b) if it does not work, then the module is unfortunately broken. Even so, an application can choose to perform the override as is done here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @FRidh. The tests are passing for python 2.7 after patching. Will make another PR after the tests also pass for 3.5, 3.6 and 3.7.

Apologies for the trouble.

Copy link
Member

Choose a reason for hiding this comment

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

Ouch, I actually knew about this, but totally missed that the change is done to a library, sorry! :/

@dhl do you need assistance to fix that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ma27 I've fixed this in #62051

@FRidh
Copy link
Member

FRidh commented May 25, 2019

We can only perform overrides of run-time dependencies in applications, not in libraries.

@dhl dhl mentioned this pull request May 25, 2019
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh FRidh left review comments

@Ma27 Ma27 Ma27 left review comments

Assignees
No one assigned
Labels
6.topic: python 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dhl @Ma27 @Mic92 @FRidh