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.

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

python-unicorn: include explicit dependency on python 2 during build #29200

Closed
wants to merge 1 commit into from
Closed

python-unicorn: include explicit dependency on python 2 during build #29200

wants to merge 1 commit into from

Conversation

vanschelven
Copy link
Contributor

... even (or rather: specifically) when building for Python 3.

As noted in the COMPILE instructions:

https://github.com/unicorn-engine/unicorn/blob/363cbacee4c54051bc9a8e717ccbbcb276c1356d/docs/COMPILE-NIX.md

Unicorn requires Python 2.x to compile.

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 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/)
  • Fits CONTRIBUTING.md.

@vanschelven
python-unicorn: include explicit dependency on python 2 during build
6935a05 
... even (or rather: specifically) when building for Python 3.

As noted in the COMPILE instructions:

https://github.com/unicorn-engine/unicorn/blob/363cbacee4c54051bc9a8e717ccbbcb276c1356d/docs/COMPILE-NIX.md

> Unicorn requires Python 2.x to compile.
@Mic92 Mic92 closed this in 8f3f64e Sep 10, 2017
Mic92 added a commit that referenced this pull request Sep 10, 2017
@Mic92
python.pkgs.unicorn: disable python3
02dd90a 
fixes #29200

(cherry picked from commit 8f3f64e)
@Mic92
Copy link
Member

Mic92 commented Sep 10, 2017

Thanks!

@vanschelven
Copy link
Contributor Author

vanschelven commented Sep 10, 2017 •
edited

Two points:

  1. The understanding that informed my original commit was that unicorn actually works well with Python 3, but happens to need Python 2 during compilation only. However, the actually merged-in commit disables Python 3 altogether. Was this intentional?

  2. The PR was against 17.09, and the cherry-picked commit that actually made it into nixpkgs was against master. It is my understanding that the latter solution is actually more correct... but this still leaves the task of cherry-picking to 17.09 open (Edit: incorrect; cherry-picking already happened)

@vanschelven
Copy link
Contributor Author

@Mic92 as per my comment above I feel that the present solution is a bit too radical, unless you have some very explicit reason to simply disable the package for python 3 altogether.

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

@FRidh FRidh Awaiting requested review from FRidh FRidh is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vanschelven @Mic92