Skip to content

pythonPackages.intelhex init at 2.1 #26581

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.

Sign up for GitHub

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
Closed
wants to merge 2 commits into from
Closed

pythonPackages.intelhex init at 2.1 #26581

wants to merge 2 commits into from
+24 −0

Conversation

pjones
Copy link
Contributor

@pjones pjones commented Jun 14, 2017

Motivation for this change

This package is a dependency of another package that I'll be submitting shortly. In other words, I need this PR approved so I can submit another one :)

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

@mention-bot
Copy link

@pjones, thanks for your PR! By analyzing the history of the files in this pull request, we identified @FRidh to be a potential reviewer.

@pjones pjones mentioned this pull request Jun 14, 2017
Merged
7 tasks
FRidh
FRidh requested changes Jun 15, 2017
View reviewed changes
Copy link
Member

@FRidh FRidh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  1. see the Nixpkgs manual on contributing Python packages.
  2. the manual and the header of this file state where expressions should go
  3. fetchPypi instead of fetchurl
  4. when disabling tests, always leave a comment explaining why.
  5. meta is missing

@pjones
pythonPackages.intelhex init at 2.1

Verified

This commit was signed with the committer’s verified signature.
NeQuissimus Tim Steinbach
GPG key ID: 472BFCCA96BD0EDA
Verified
Learn about vigilant mode
e1096f5
@pjones pjones force-pushed the pjones/intelhex branch from 3d77946 to e1096f5 Compare June 15, 2017 16:07
@pjones
Copy link
Contributor Author

pjones commented Jun 15, 2017

@FRidh Sorry about that, I got super lazy on this one. It's (hopefully) fixed now.

@joachifm joachifm added the 8.has: package (new) This PR adds a new package label Jun 16, 2017
@Mic92
intelhex: enable tests

Verified

This commit was signed with the committer’s verified signature.
fpletz Franz Pletz
GPG key ID: 846FDED7792617B4
Verified
Learn about vigilant mode
20acfd5 
(they seem to work for all somehow)
Mic92
Mic92 reviewed Jun 18, 2017
View reviewed changes
pkgs/development/python-modules/intelhex/default.nix Outdated
# Harmless test failure:
# https://github.com/bialix/intelhex/issues/14
doCheck = false;

Copy link
Member

Choose a reason for hiding this comment

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

Lets see if travis confirms it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's my understanding that the test will only fail on 64-bit systems. Please keep that in mind.

@FRidh
Copy link
Member

FRidh commented Jun 23, 2017

pushed aa3f37c

@FRidh FRidh closed this Jun 23, 2017
@pjones
Copy link
Contributor Author

pjones commented Jun 27, 2017

@FRidh The sha256 you used for the patch is bad:

output path ‘/nix/store/b0rbpdjhacn5p96h91hwan2pm9hf67h7-f251aef214daa2116e15ff7f7dcec1639eb12d5b.patch’ has sha256 hash ‘1fiqbha923idibjzdyq9p6xrjwak36yr02pzb7dg8lf1mv0w1cjc’ when ‘02i15qjmcz7mwbwvyj3agl5y7098rag2iwypdilkaadhbslsl9b9’ was expected

@FRidh
Copy link
Member

FRidh commented Jun 27, 2017

ugh that's the last time I use fetchpatch.
Fixed in f2b97e0

@pjones
Copy link
Contributor Author

pjones commented Jun 27, 2017

Thanks for the quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92

@FRidh FRidh

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pjones @mention-bot @FRidh @Mic92 @joachifm