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

pythonPackages.cfn-lip: init at 1.1.0 #52944

Merged
merged 1 commit into from Feb 7, 2019
Merged

pythonPackages.cfn-lip: init at 1.1.0 #52944

merged 1 commit into from Feb 7, 2019

Conversation

PsyanticY
Copy link
Contributor

@PsyanticY PsyanticY commented Dec 26, 2018 •
edited

Motivation for this change
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.

@PsyanticY PsyanticY force-pushed the cfn_flip branch 3 times, most recently from a139cef to 51e4015 Compare December 27, 2018 19:50
dotlambda
dotlambda requested changes Jan 26, 2019
View reviewed changes
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/development/tools/cfn_flip/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/cfn_flip/default.nix Outdated Show resolved Hide resolved
@PsyanticY
Copy link
Contributor Author

@dotlambda Thanks for the review. addressed requested changes,
for pytestunner it is required in the package setup.py

@dotlambda
Copy link
Member

for pytestunner it is required in the package setup.py

Then it should not be propagated. @FRidh I'm still not whether to use buildInputs or propagatedBuildInputs in that case.

dotlambda
dotlambda requested changes Jan 29, 2019
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Judging from their README it looks this should be moved into python-packages.nix. Shouldn't it?

@PsyanticY
Copy link
Contributor Author

@dotlambda I think nativeBuildInputs would do the job ?
I think it should not be in python-packages.nix, since most of the people would use it as a binary to convert form Json to yaml and not as a package in other python code. But, it is up to you and @FRidh to decide.

@dotlambda
Copy link
Member

There's no harm in putting it in python-packages.nix and since it can be imported as a Python library it should be put there.

@PsyanticY PsyanticY requested a review from FRidh as a code owner February 4, 2019 16:37
@PsyanticY
Copy link
Contributor Author

@dotlambda Moved the package to python-packages.

@dotlambda dotlambda changed the title cfn_flip: init at 1.1.0 pythonPackages.cfn-lip: init at 1.1.0 Feb 7, 2019
dotlambda
dotlambda requested changes Feb 7, 2019
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Please set the commit message to this PR's title.

pkgs/top-level/python-packages.nix Outdated
@@ -303,6 +303,8 @@ in {
cleo = callPackage ../development/python-modules/cleo { };

clikit = callPackage ../development/python-modules/clikit { };

cfn_flip = callPackage ../development/python-modules/cfn_flip { };
Copy link
Member

Choose a reason for hiding this comment

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

f comes before l if I remember correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah right hhhhhhh @dotlambda

Copy link
Member

Choose a reason for hiding this comment

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

still wrong, as is the commit message
and I forgot to tell you to normalize attribute and filename to cfn-flip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah seems like i didn't git add .

@PsyanticY
Copy link
Contributor Author

@dotlambda fixed that ^^

@PsyanticY
Copy link
Contributor Author

@dotlambda should be good now.

@dotlambda
Copy link
Member

@GrahamcOfBorg build python2.pkgs.cfn-flip python3.pkgs.cfn-flip

@dotlambda dotlambda merged commit 297c931 into NixOS:master Feb 7, 2019
@PsyanticY PsyanticY deleted the cfn_flip branch February 7, 2019 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotlambda dotlambda dotlambda requested changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 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
3 participants
@PsyanticY @dotlambda @GrahamcOfBorg