-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
NixOS friendly build process #683
Conversation
1. keep given PYTHONPATH around (after our pip install dir) 2. build binaries rather than downloading
worker/Makefile
Outdated
@@ -42,7 +42,7 @@ ifeq ($(wildcard $(PIP_DIR)),) | |||
$(PYTHON) -m pip install --target=$(PIP_DIR) pip setuptools || \ | |||
echo "Installation failed, likely because PIP is unavailable, if you are on Debian/Ubuntu or derivative please install the python3-pip package" | |||
# Install `meson` and `ninja` using `pip` into custom location, so we don't depend on system-wide installation. | |||
$(PYTHON) -m pip install --upgrade --target=$(PIP_DIR) meson ninja | |||
$(PYTHON) -m pip install --upgrade --target=$(PIP_DIR) --no-binary ":all:" meson ninja |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary though, can you provide an error output?
Also I believe this deserves a comment on why are we not using binaries.
BTW, you might be able to run command as is and fall back to compilation only when it fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NixOS doesn't have the standard /usr
directories but has per-package bundles in a content-addressable store. NixOS tweaks build processes to set the binary's RPATH
to deal with this: and so binaries compiled elsewhere almost certainly won't work.
Could try the fallback thing - would need to do ldd <binary>
and check for not found
on a list of binaries.
Could also check explicitly for NixOS (existence of /etc/NIXOS
); would need to do this for guix
as well (and maybe others?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just do x || y || z
and add a comment why we have the alternative option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will successfully create a ninja binary that doesn't work (on NixOS, GUIX)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, you're right. Are NixOS folks aware of broken Python dependencies because of this at all? Maybe you can provide a link to the relevant issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might have not been very clear. I suggested to report Python issue, maintainers of those distributions better make --no-binary ":all:"
the default behavior of packages Python version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I misunderstood. I will pass this very sensible suggestion on to the NixOS maintainers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will you be able to update this PR to have a check for NixOS and only build stuff on it (with comment to upstream issue so we can remove it in the future when NixOS fixes this problem)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. Is make
guaranteed to be GNU Make? (it uses GNUisms at the moment, I think)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we target GNU Make right now
This patch may be removed if NixOS/nixpkgs#142383 and a similar change to guix is actioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but I would still like to see link to upstream report about this
Thank you, this will be included into the next release |
I'm not particularly expert in NixOS and there may be a better way of doing this, but at least these changes are minimal. There is a performance hit for 2 but not much as a fraction of the overall build.
Tested on debian testing and NixOS 21.05