-
Notifications
You must be signed in to change notification settings - Fork 102
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
Resolve build problems in 3.3.1.0 #1026
Conversation
This fixes the 1st issue described in juju#1025 (comment)
file This fixes juju#1025 by removing the dependency to the external static VERSION file from within the project, allowing it to work in environments where pylibjuju is installed as a dependency library. Note that this changes the release process. In particular where we need to manually change the version information is moved into version.py (the release process document will be updated) This also removes the VERSION file.
|
||
# If extensions (or modules to document with autodoc) are in another directory, | ||
# add these directories to sys.path here. If the directory is relative to the | ||
# documentation root, use os.path.abspath to make it absolute, like shown here. | ||
sys.path.insert(0, os.path.abspath('..')) | ||
|
||
from juju.version import CLIENT_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.
Imports should be at the top no?
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.
This one should stay there because it won't work before the sys.path.insert(0, os.path.abspath('..'))
line above
|
||
setup( | ||
name='juju', | ||
version=version.read_text().strip(), | ||
version=CLIENT_VERSION.strip(), |
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.
Do we still need to strip?
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.
We currently don't need to, but the line that's coming from will be manually edited by people who're making releases (replaces the file VERSION) in the future, so I thought this would cleanup any silly mistakes like "4.2.0 " (whitespace in the end).
/merge |
Description
This solves 2 problems with the latest release (also described in this comment):
packaging
module, but didn't include it in thesetup.py
(because our testing and dev environments have tox installed and tox satisfies that requirement for itself, so we didn't notice).This change fixes #1025, solving these issues by:
packaging
to the required modules.VERSION
, and moving the static version information into theversion.py
module (just like in Juju) and use that information from there. The places that dynamically use that information (outside of the juju modules) are i)setup.py
to get the version info into the build, ii)Makefile
to use it in the release target iii)docs/config.py
for building the docs. All of these spots are updated to use the info within theversion.py
.QA Steps
We'll use the steps described in #1025. After pulling the branch in this PR;
dist/juju-3.3.1.0.tar.gz
, so install that:Controller
.Without the (1)st change in this PR, this command would result in:
And without the (2)nd change, this command would error with
But with this change, we shouldn't see either (or any other) error. I.e., the command should succeed with no problems.
Notes & Discussion
Note that this changes the release process. In particular where we need to manually change the version information is moved into the
version.py
module (the release process document will be updated right before this PR is merged).Moreover, we need to make a patch release (
3.3.1.1
) after this for these changes to take effect (i.e. for pylibjuju to be usable in the environments where it's installed as a dependency).JUJU-5484