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.

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

ci: add current git sha to version in build-wheel job #856

Merged
merged 2 commits into from
Dec 5, 2023

Conversation

afuetterer
Copy link
Member

@afuetterer afuetterer commented Dec 2, 2023

Description

This PR proposes the following changes:

  • remove the obsolote VERSION variable from init.py
  • add the current git SHA to the version in build-wheel job

Motivation and Context

As discussed in the slack channel yesterday, this was desired by @jochenklar.

Originally the build-wheel job was introduced in #812.

It created a python wheel (including the JavaScript frontend code bundled with webpack) under the name

rdmo-{version}dev{pr_number}-py3-none-any.whl

e.g. given
version = 2.0.2 and pr_number = 833
rdmo-2.0.2dev833-py3-none-any.whl

@jochenklar suggested the new wheel name to include the git SHA (7 chars short version) to ease deploying multiple times from the same PR (possibly long living like the dev2.1.0).

However the suggested form of
rdmo-{version}.dev{sha}-py3-none-any.whl
is invalid, due to PEP440 and Version specifiers.

dev can only be followed by an integer, which the PR number was.

I chose this new pattern instead:

rdmo-{version}.dev0+{sha}-py3-none-any.whl

which is valid. Another possibility would be:

rdmo-{version}+{sha}-py3-none-any.whl

What do you think?

How has this been tested?

Tested locally.

$ python -m pip install rdmo-2.0.2.dev0+553c22a-py3-none-any.whl
$ pip show rdmo                                       
Name: rdmo
Version: 2.0.2.dev0+553c22a
Summary: RDMO is a tool to support the systematic planning, organisation and implementation of the data management throughout the course of a research project.
Home-page: 
Author: 
Author-email: RDMO Arbeitsgemeinschaft <[email protected]>
License: Apache-2.0
Location: /home/afuetterer/workspace/github/rdmo-fork/.venv/lib/python3.12/site-packages
Requires: defusedcsv, defusedxml, django, django-cleanup, django-compressor, django-extensions, django-filter, django-libsass, django-mathfilters, django-mptt, django-rest-swagger, django-settings-export, django-split-settings, django-widget-tweaks, djangorestframework, drf-extensions, iso8601, markdown, pypandoc, requests-toolbelt, rules
Required-by: 

Types of Changes

  • Other (please describe):

Checklist

  • I have read the contributor guide.
  • My code follows the code style of this project.
  • All new and existing tests passed.

Tasks

  • update version regex to select string between quotes
  • remove dev+0
  • write version to github step summary
  • use pr head sha

@afuetterer

This comment was marked as resolved.

@jochenklar
Copy link
Member

Hmm, you are right, we had something like rc1 before. I don't know if we will have that any time soon, since this was mainly to create a version on PyPI. Now that we have the wheels this is not needed anymore to deploy a development version.

I think I like the +1234567 with out the dev part more. It would also work if there is a more complicated version already in rdmo/__init__.py. I don't think that the PR number needs to be in the filename. But maybe @MyPyDavid will disagree.

I we can keep __version__ the only thing in __init__.

@afuetterer
Copy link
Member Author

If the version is the only thing in ini.py, I will adapt the replace the current version with the suffixed version. Then rc, dev, etc, is not going to be a problem.

I can remove the middle part .dev0+.

Let's hear from @MyPyDavid about the PR number.

I will adapt this PR, as soon as all agree.

@afuetterer

This comment was marked as resolved.

@MyPyDavid
Copy link
Member

Think that the commit {sha} will then be sufficient, it's unique and findable.
We could change the version in __init__.py to be similar as they do in django https://github.com/django/django/blob/main/django/__init__.py ?

@jochenklar
Copy link
Member

You mean using the get_version method? I don't think that helps much, it will make it probably harder to patch it in the CI. (They have the VERSION btw., still not sure why.)

@afuetterer

This comment was marked as resolved.

@jochenklar
Copy link
Member

just +{sha}, maybe we will use dev1... in the future manually.

@MyPyDavid

This comment was marked as resolved.

@afuetterer afuetterer force-pushed the build-wheel branch 2 times, most recently from 675a487 to 4465666 Compare December 4, 2023 18:21
@afuetterer
Copy link
Member Author

afuetterer commented Dec 4, 2023

All done, please check.

The wheel is built, the metadata is checked and then I install rdmo (without dev etc) from this wheel. If all works without error, the wheel is uploaded is an artifact.

Should be safe to use in a deployment.

I added a summary message for the build-wheel step. It will show the output of "pip show rdmo", which includes the "new" version (including SHA).
See: https://github.com/rdmorganiser/rdmo/actions/runs/7090853761#summary-19299866596

@afuetterer afuetterer requested a review from MyPyDavid December 4, 2023 18:23
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Copy link
Member

@MyPyDavid MyPyDavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super, thank you!

@afuetterer afuetterer merged commit 47bdc2c into rdmorganiser:dev-2.1.0 Dec 5, 2023
12 checks passed
@afuetterer afuetterer deleted the build-wheel branch December 5, 2023 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants