-
Notifications
You must be signed in to change notification settings - Fork 187
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
🔧 Move to flit for PEP 621 compliant package build #5312
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
afd74c7
🔧 Move to flit for package build
chrisjsewell 63e28f0
Update .pre-commit-config.yaml
chrisjsewell 25c6a25
update dependency checks
chrisjsewell 5e22916
Update requirements.txt
chrisjsewell f575508
Update dependency_management.py
chrisjsewell 3eb9224
replace occurrences of setup.json
chrisjsewell 5ad0043
Update release.yml
chrisjsewell b8da438
Merge branch 'develop' into flit
chrisjsewell 2071695
Update pyproject.toml
chrisjsewell 8322c98
Merge branch 'develop' into flit
chrisjsewell 409599b
Update release.yml
chrisjsewell 1dccb26
Update utils/requirements.txt
chrisjsewell a025d01
Merge branch 'develop' into flit
chrisjsewell a2f57cd
remove arg from check_release_tag.py
chrisjsewell File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,31 @@ | ||
# -*- coding: utf-8 -*- | ||
"""Check that the GitHub release tag matches the package version.""" | ||
import argparse | ||
import json | ||
import ast | ||
from pathlib import Path | ||
|
||
|
||
def get_version_from_module(content: str) -> str: | ||
"""Get the __version__ value from a module.""" | ||
# adapted from setuptools/config.py | ||
try: | ||
module = ast.parse(content) | ||
except SyntaxError as exc: | ||
raise IOError(f'Unable to parse module: {exc}') | ||
try: | ||
return next( | ||
ast.literal_eval(statement.value) for statement in module.body if isinstance(statement, ast.Assign) | ||
for target in statement.targets if isinstance(target, ast.Name) and target.id == '__version__' | ||
) | ||
except StopIteration: | ||
raise IOError('Unable to find __version__ in module') | ||
csadorf marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
if __name__ == '__main__': | ||
parser = argparse.ArgumentParser() | ||
parser.add_argument('GITHUB_REF', help='The GITHUB_REF environmental variable') | ||
parser.add_argument('SETUP_PATH', help='Path to the setup.json') | ||
args = parser.parse_args() | ||
assert args.GITHUB_REF.startswith('refs/tags/v'), f'GITHUB_REF should start with "refs/tags/v": {args.GITHUB_REF}' | ||
tag_version = args.GITHUB_REF[11:] | ||
with open(args.SETUP_PATH, encoding='utf8') as handle: | ||
data = json.load(handle) | ||
pypi_version = data['version'] | ||
assert tag_version == pypi_version, f'The tag version {tag_version} != {pypi_version} specified in `setup.json`' | ||
pypi_version = get_version_from_module(Path('aiida/__init__.py').read_text(encoding='utf-8')) | ||
assert tag_version == pypi_version, f'The tag version {tag_version} != {pypi_version} specified in `pyproject.toml`' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 take it that you employ this (convoluted) way of reading the version number because you cannot import it normally? Instead of this approach, is there no simple way to make the package importable so we can just do
At the very least, I think it would be useful to have a comment here explaining why this approach is necessary and the import way doesn't work or is not desirable
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 wouldn't call this approach particularly convoluted; I would say having to go through the whole process of installing/importing the package is convoluted, just to read the version from a file 🤷
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.
Feel free to create a
suggestion
for what the docstring should be (and I'll happily merge), but I'm not adding one myself, because I don't feel it needs it, since this IMO is the canonical way to retrieve the__version__
attribute (it is how both setuptools and flit do it)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.
If generating an abstract syntax tree, parsing it element by element just to find the one string that defines the version number is not convoluted, then I don't know what is. I can see the point that having to install it is likewise a lot of work just to get the version, but at least it is very understandable and readable. One would think that there should be an easier way to do this... But in the absence of that, guess this is fine to keep
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.
@sphuber It is convoluted, but to my understanding, one main motivation for the overall change is to make the project metadata and build definition more declarative, so needing to install and import the package runs exactly counter that. I believe that we can reduce complexity by either parsing the module directly with a regex or by moving to a different version specification like I suggested in my comment. My recommendation would be to move forward and avoid bikeshedding on this 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 already agreed to keeping this and move forward. Still, the suggestion to add a comment explaining why this is being done doesn't seem like a big or weird ask. Even if this may be the standard, I doubt many developers are aware of this and they will almost certainly wonder why it has to be (or look) so complicated.
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 would note, that on the next line, it says:
# adapted from setuptools/config.py
, but yeh honestly this is just a very simple check (that wasn't even here a year ago) to make sure the release tag is consistent with the package version