Skip to content

Update default setuptools_scm version scheme to release-branch-semver - #82

Merged
chrisjbillington merged 2 commits into
labscript-suite:masterfrom
dihm:setuptools_scm-fix
Apr 9, 2021
Merged

Update default setuptools_scm version scheme to release-branch-semver#82
chrisjbillington merged 2 commits into
labscript-suite:masterfrom
dihm:setuptools_scm-fix

Conversation

@dihm

@dihm dihm commented Dec 29, 2020

Copy link
Copy Markdown
Contributor

This fixes the circular dependency issue between blacs and labscript-devices that is preventing #80 from building. By extension, allows for proper pip develop installs locally, where this circular dependency issue is manifesting.

Pretty sure I got all the tweaks from labscript-utils, but best to make sure I didn't miss anything, especially where the workflows/RTD stuff is concerned since that is harder to test locally.

This should be merged at same time as labscript-suite/labscript-devices#73

@philipstarkey

Copy link
Copy Markdown
Member

Great! I think the only thing missing (that is in labscript_utils) is the specific version in setup.cfg for setuptools_scm

@philipstarkey

Copy link
Copy Markdown
Member

Note that labscript-suite/labscript-suite#55 is sort of a blocker for this as our GitHub builds are completely broken at the moment. I'm uncertain off the top of my head how this interacts with readthedocs builds. It probably doesn't break them as I don't think readthedocs relies on development builds on labscript dependencies...but it would be preferable to merge this after fixing the build issues so we can make sure this doesn't introduce separate build issues!

@dihm

dihm commented Dec 30, 2020

Copy link
Copy Markdown
Contributor Author

Great! I think the only thing missing (that is in labscript_utils) is the specific version in setup.cfg for setuptools_scm

I actually noticed that one while working on this and forgot to bring it up. I was going to add the change as well but noticed that both labscript-devices and blacs depend on labscript-utils (which in turn depends on the correct version of setuptools-scm) and figured I would leave it be. I presume I should favor more explicit version requirements in each package over relying on chains of dependencies?

Note that labscript-suite/labscript-suite#55 is sort of a blocker for this as our GitHub builds are completely broken at the moment.

Sure thing! I think I agree that the RTD builds are still nominally OK, but the high level github builds magic is fairly new territory for me.

dihm added 2 commits April 2, 2021 13:51
This fixes the circular dependency issue between blacs and labscript-devices.
Adds setuptools-scm with required version.
@dihm
dihm force-pushed the setuptools_scm-fix branch from d93abde to 75b10aa Compare April 2, 2021 17:52
@dihm

dihm commented Apr 2, 2021

Copy link
Copy Markdown
Contributor Author

Rebased onto master to pick up fix from #83. Also added the explicit version requirement on setuptools-scm.

Comment thread pyproject.toml
@@ -1,3 +1,3 @@
[build-system]
requires = ["setuptools", "wheel", "setuptools_scm"]
requires = ["setuptools>=42", "wheel", "setuptools_scm[toml]>=4.1.0"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dihm What's the reason for the toml requirement?

Seems to me that it shouldn't be necessary since setuptools_scm only uses toml to read its configuration from pyproject.toml. Since we still need to set its config in setup.py based on an environment variable, we aren't using that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, looks like you were probably copying from the recommended usage here: https://pypi.org/project/setuptools-scm/

including the requirement to use setuptools > 42.

I'm going to leave these changes out for simplicity.

@chrisjbillington
chrisjbillington merged commit 75b10aa into labscript-suite:master Apr 9, 2021
@chrisjbillington

Copy link
Copy Markdown
Member

Sorry for the slowness on this. I've merged it, but with a few changes:

  • Not specifying [toml] or setuptools>=42 in pyproject.toml
  • Still using an environment variable for the local version in __version__.py - this should be consistent with the setup.py version which still uses the environment variable, in case the module actually gets imported in CI we would want this to agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants