tomMoral commented 3 years ago

I create the PR as @deepcharles might be able to help us with the automatic version update.

codecov[bot] commented 3 years ago

Codecov Report

Merging #31 (01a6d4e) into main (fa0be0a) will increase coverage by 0.34%. The diff coverage is 93.75%.

@@            Coverage Diff             @@
##             main      #31      +/-   ##
==========================================
+ Coverage   74.18%   74.53%   +0.34%     
==========================================
  Files          41       43       +2     
  Lines        2572     2611      +39     
==========================================
+ Hits         1908     1946      +38     
- Misses        664      665       +1

Flag	Coverage Δ
unittests	`74.53% <93.75%> (+0.34%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dicodile/config.py	`100.00% <ø> (ø)`
dicodile/data/_text.py	`0.00% <0.00%> (ø)`
dicodile/__init__.py	`100.00% <100.00%> (ø)`
dicodile/_dicodile.py	`79.51% <100.00%> (+0.24%)`	:arrow_up:
dicodile/data/gait.py	`100.00% <100.00%> (ø)`
dicodile/data/images.py	`76.92% <100.00%> (+12.21%)`	:arrow_up:
dicodile/data/tests/test_gait.py	`100.00% <100.00%> (ø)`
dicodile/data/tests/test_images.py	`100.00% <100.00%> (ø)`
dicodile/tests/test_config.py	`100.00% <100.00%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fa0be0a...01a6d4e. Read the comment docs.

rprimet commented 3 years ago

We were on the brink of submitting it (although it needs restricting to pushes to main branch).

We were just concerned about making sure that an automated release does not happen if unit tests (or other critical steps in other workflows) fail.

But maybe having a manual trigger for release (i.e. a push-button action in the github UI) would let us sidestep this issue?

deepcharles commented 3 years ago

To avoid in-source versionning without changing the current github action, you could use setuptools_scm to automatically create a version.py file that contains the version number (in a __version__ variable).

The version number is computed from the latest tag (defaults to 0.0.0 I think), the distance from this tag, and the directory state (e.g. uncommitted changes since latest tag).

You could also add the following snippet to your github action to manually trigger the upload to Pypi, with a specific version number:

on:
  workflow_dispatch:
    inputs:
      overrideVersion:
        description: Manually force a version
env:
  SETUPTOOLS_SCM_PRETEND_VERSION: ${{ github.event.inputs.overrideVersion }}

This is particularly useful if you want to push a release candidate before an actual release.

deepcharles commented 3 years ago

You get this interface to trigger an upload.

rprimet commented 3 years ago

The version number is computed from the latest tag (defaults to 0.0.0 I think), the distance from this tag, and the directory state (e.g. uncommitted changes since latest tag).

So this is a bit similar to versioneer then?

deepcharles commented 3 years ago

Yes, this is very similar.

Two differences that make it a better tool in my opinion:

setuptools_scm is recommended by pypa (and the devs of setuptools);
with setuptools_scm, you can manually force a version, and (I am pretty sure) it is not possible with versioneer.

rprimet commented 3 years ago

@tomMoral @agramfort @deepcharles @hndgzkn since we're overhauling build files, should we attempt to move as much metadata as possible into setup.cfg?

tomMoral commented 3 years ago

@tomMoral @agramfort @deepcharles @hndgzkn since we're overhauling build files, should we attempt to move as much metadata as possible into setup.cfg?

That would be nice!

agramfort commented 3 years ago

+1 for setup.cfg

rprimet commented 3 years ago

Still needs to be discussed / solved:

For now there's no explicit dependency of the release workflow on the lint and unit tests workflows. I'm not sure what the best solution is:

We could merge all workflows in the same file and specify a dependency between jobs, but in addition of being cumbersome, the workflow trigger might not have the right specification, i.e. we might want to perform linting and unit tests on every push to any branch, while the release process should be limited to pushes to main for testpypi and tags for pypi.
We could omit any explicit dependency and rely on the assumption that we will not tag a commit that did not pass CI? (plus maybe a quick sanity check, like running pytest before the build)
We could trigger the release workflow on completion of the unittest workflow to avoid merging files, but that would not place a dependency on the linting and the trigger condition issue would remain...

Any thoughts?

tomMoral commented 3 years ago

Thx a lot @rprimet ! Merging this one, let's see how it goes and update if we find something that is not working.

rprimet commented 3 years ago

Looks like the merge did not trigger a release to testpypi, so there's probably something wrong in the testpypi if guard?... I'll have a look.

rprimet commented 3 years ago

Nevermind, the pipeline runs twice (once for event pull_request and one for push) and the push pipeline did trigger a testpypi release.

tomMoral / dicodile

Automatic build and publish action #31

Codecov Report