-
Notifications
You must be signed in to change notification settings - Fork 337
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
Install as package part2 #845
Conversation
Looks good, maybe you can add some Ci/tests to it, like the ones from #579 (which I could close in favour of this one) |
Hey Joe,
Yeah I agree. It's particularly good to have the tests run via the rez
package, as this will pick up any inconsistencies between that and the
production installation.
There's two things I think we need to address first though:
- the python rez-bind. I'd really like to get rid of this. I wonder if we
should create the rez package in a special mode (for this workflow) that
ditches the python requirement. The end result would be the same (since the
rez-bind interfaces with system python anyway) but without the bind stuff
(which we want to deprecate). We could achieve this by having an env-var
that the workflow sets, which then causes the created rez package to drop
the python requirement.
- the rez package has no cli tools (by design as mentioned in PR), so we
need to expose a way to run the tests via API, but that seems reasonable to
do anyway and should be trivial.
What are your thoughts? What I'm thinking is we put your PR "ON HOLD", and
ticket up something to add the changes as outlined above, and I'l merge
this new PR in the meantime. I want to keep your workflow around for
reference until we sort this out.
Cheers
A
…On Fri, Feb 7, 2020 at 10:47 AM Joe Yu ***@***.***> wrote:
Looks good, maybe you can add some Ci/tests to it, like the ones from #579
<#579> (which I could close in
favour of this one)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#845>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMOUSV5TF53PEAACNFE66LRBSOP3ANCNFSM4KORGK6A>
.
|
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.
Looking at it more, I think what I was trying to achieve with #579
might be different than in this PR:
-
This PR looks to me to be focusing more just the
rez
andrezplugin
Python modules, similar torez bind rez
-
[Feature] Install as package #579 was trying to do a production install of rez and as a rez package,
similar topython install.py DIR
and then makingDIR
a rez package.- Adds
rez
production install scripts toPATH
, and - Adds
rez
production install'srez
andrezplugin
toPYTHONPATH
- Adds
I feel this PR should be prioritised though as I like the new functions and
installer.py
introduced here. #579 can then be rebased and refactored to use
these.
I'm happy to have my my PR on hold while this is being merged.
# cleanup temp install | ||
try: | ||
shutil.rmtree(tmpdir) | ||
except: |
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.
except: | |
except Exception: |
def install_as_rez_package(repo_path): | ||
"""Installs rez as a rez package. | ||
|
||
Note that this can be used to install new variants of rez into an existing | ||
rez package (you may require multiple platform installations for example). | ||
|
||
Args: | ||
repo_path (str): Full path to the package repository to install into. | ||
""" | ||
from tempfile import mkdtemp | ||
|
||
# do a temp production (venv-based) rez install | ||
tmpdir = mkdtemp(prefix="rez-install-") | ||
install(tmpdir) | ||
|
||
try: |
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.
Maybe this temp install setup can be turned into a context so it can be re-used in the future:
temp_install context
import contextlib
@contextlib.contextmanager
def temp_install(cleanup=True, print_welcome=False):
"""Do a temp production (venv-based) rez install.
Args:
cleanup (bool): Cleanup temp install when exiting context.
"""
from tempfile import mkdtemp
tmpdir = mkdtemp(prefix="rez-install-")
install(tmpdir, print_welcome=print_welcome)
try:
yield tmpdir
finally:
if cleanup:
try:
shutil.rmtree(tmpdir)
except Exception:
pass
def install_as_rez_package(repo_path):
"""Installs rez as a rez package.
Note that this can be used to install new variants of rez into an existing
rez package (you may require multiple platform installations for example).
Args:
repo_path (str): Full path to the package repository to install into.
"""
with temp_install() as tmpdir:
# This extracts a rez package from the installation. See
# rez.utils.installer.install_as_rez_package for more details.
#
args = (
os.path.join(tmpdir, "bin", "python"), "-E", "-c",
r"from rez.utils.installer import install_as_rez_package;"
r"install_as_rez_package('%s')" % repo_path
)
print(subprocess.check_output(args))
Working off #579, but with following changes (@j0yu ):