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

pybitmessage: 0.4.4 -> 0.6.2 #26616

Closed
wants to merge 3 commits into from
Closed

Conversation

photm5
Copy link
Contributor

@photm5 photm5 commented Jun 16, 2017

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

It seems kinda strange to me to have pybitmessage in instant-messengers (since it is not meant to be used that way, but rather asynchronously), I think it would fit better in p2p. Should I change that too?

@photm5
Copy link
Contributor Author

photm5 commented Jun 16, 2017

Oh, forgot to mention: I only built this on 451478c and 32bit, since I don't have a 64bit machine at hand and it wants to recompile systemd etc. on master.

@photm5 photm5 changed the title Pybitmessage 0.6.2 pybitmessage: 0.4.4 -> 0.6.2 Jun 16, 2017
@@ -30,7 +30,7 @@ patchPythonScript() {

# The magicalSedExpression will invoke a "$(basename "$f")", so
# if you change $f to something else, be sure to also change it
# in pkgs/top-level/python-packages.nix!
# in wrap-python.nix!
# It also uses $program_PYTHONPATH.
sed -i "$f" -re '@magicalSedExpression@'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix the broken quoting in wrap.sh in various places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate some more? My intention was only to fix the broken reference to wrap-python.nix, nothing else. If there's more to be done in wrap.sh, it should probably not be in this PR, since fixing that reference is already extraneous to this PR, which is about bitmessage.

Copy link
Member

Choose a reason for hiding this comment

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

just ignore this comment.

--replace "PREFIX?=/usr/local" "" \
--replace "/usr" ""
'';
substituteInPlace setup.py \
Copy link
Member

Choose a reason for hiding this comment

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

Why are you making these replacements? Add a comment explaining what its for.

--replace "/opt/openssl-compat-bitcoin/lib/" "${openssl.out}/lib/"
wrapProgram $out/bin/pybitmessage \
--prefix PYTHONPATH : "$(toPythonPath $out):$PYTHONPATH"
makeWrapperArgs = ''
Copy link
Member

Choose a reason for hiding this comment

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

Patching the source to hardcode paths to libraries has preference over setting LD_LIBRARY_PATH.

@@ -30,7 +30,7 @@ patchPythonScript() {

# The magicalSedExpression will invoke a "$(basename "$f")", so
# if you change $f to something else, be sure to also change it
# in pkgs/top-level/python-packages.nix!
# in wrap-python.nix!
Copy link
Member

Choose a reason for hiding this comment

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

thanks!

@FRidh
Copy link
Member

FRidh commented Jul 17, 2017

ping me when its done and I'll merge

Actually, the 'print…' substitutions wouldn't be necessary, but since
the output confused me while packaging, I thought I'd protect others
from wasting their precious time on the same matter.
@photm5
Copy link
Contributor Author

photm5 commented Jul 19, 2017 via email

@FRidh
Copy link
Member

FRidh commented Jul 19, 2017

It would help if you show what you tried.

@photm5
Copy link
Contributor Author

photm5 commented Jul 20, 2017 via email

mguentner pushed a commit to mguentner/nixpkgs that referenced this pull request Sep 1, 2017
mguentner added a commit to mguentner/nixpkgs that referenced this pull request Sep 1, 2017
@mguentner mguentner mentioned this pull request Sep 1, 2017
8 tasks
@Mic92 Mic92 closed this in #28824 Sep 1, 2017
Mic92 pushed a commit that referenced this pull request Sep 1, 2017
squashed hashes:
3ee20b2
b9a3a3b

(details -> #26616)

(cherry picked from commit e3d7c4c)
Mic92 pushed a commit that referenced this pull request Sep 1, 2017
fixes requested changes in #26616

(cherry picked from commit e920377)
@photm5
Copy link
Contributor Author

photm5 commented Sep 9, 2017

@FRidh Sorry for the rough tone I used in my replies. I got fairly emotional (but I'm not sure how much my replies are showing that) given I didn't agree that the changes you requested would be useful. Now that I know (e920377) how easy it would've been (which I kinda assumed), I think your "nitpicking" wasn't that bad. Thanks for your work as a maintainer!

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

5 participants