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

azure-storage-python #54261

Merged
merged 5 commits into from Jan 22, 2019
Merged

azure-storage-python #54261

merged 5 commits into from Jan 22, 2019

Conversation

CMCDragonkai
Copy link
Member

@CMCDragonkai CMCDragonkai commented Jan 18, 2019

Motivation for this change

This is a set of different packages all from https://github.com/Azure/azure-storage-python

Note that while the set ends up working in Python 3. In Python 2, they require an extra dependency of futures. See the setup.py of each subpackage. I'm not sure how to make this work.

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 nox --run "nox-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.

@CMCDragonkai
Copy link
Member Author

Not sure whether this should be separate commits, or 1 single commit?

@CMCDragonkai
Copy link
Member Author

How to resolve the setup.py condition on Python 2?

    extras_require={
        ":python_version<'3.0'": ['futures'],
    },

@dotlambda
Copy link
Member

Not sure whether this should be separate commits, or 1 single commit?

Separate ones. And they should have pythonPackages in the commit message.

How to resolve the setup.py condition on Python 2?

+ lib.optional (!isPy3k) futures

@CMCDragonkai
Copy link
Member Author

Where do I get isPy3k from?

@dotlambda
Copy link
Member

Where do I get isPy3k from?

as an argument

@CMCDragonkai
Copy link
Member Author

I'm going to use lib.optional. However I realised that there's a dependency graph here and thankfully not mutually dependent. So there's going to be an order to these commits too.

Just a note lib.optionals allows a list of dependencies.

@CMCDragonkai
Copy link
Member Author

How do I do this? "Determined the impact on package closure size (by running nix path-info -S before and after)"

@dotlambda Edits are all done.

@CMCDragonkai CMCDragonkai changed the title azure-storage-python: init at 1.4.0 azure-storage-python Jan 21, 2019
@CMCDragonkai
Copy link
Member Author

I changed PR name.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Jan 21, 2019

@dotlambda Strange, how the commit log is on a different order to this PR's commits. I committed azure-storage-nspkg first, but it appears that Github bot is testing the last commit first.

Related to: https://stackoverflow.com/questions/44704519/how-to-fix-commit-order-in-github-pull-requests-broken-by-git-rebase

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

I don't see any tests for these packages so please disable the check phases with

doCheck = false;

@worldofpeace
Copy link
Contributor

Aside from that these all build for me locally.

@CMCDragonkai CMCDragonkai force-pushed the azure-storage branch 2 times, most recently from b24261d to a412123 Compare January 22, 2019 04:14
@CMCDragonkai
Copy link
Member Author

All edits done, still don't quite understand why the commit order is different though.

@worldofpeace
Copy link
Contributor

@CMCDragonkai Sorry, I forgot to tell that we need a comment at every doCheck = false; like

# Has no tests

I'm always forget that one 😄

@CMCDragonkai
Copy link
Member Author

Done.

@worldofpeace worldofpeace merged commit a5de410 into NixOS:master Jan 22, 2019
@worldofpeace
Copy link
Contributor

Thanks again 👍

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