-
Notifications
You must be signed in to change notification settings - Fork 179
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
chore(build): Derive application versions from git #11863
Conversation
This commit updates RELEASING.md to the new process for running releases, which are now based entirely on pushing tags - no more manual changelog generation and trimming, no more bump prs since the version is not checked in anywhere. This drastically reduces the sheer number of steps, number of prs, and number of builds to wait for.
Remove lerna as a top-level dep and put placeholder versions in all package.json
The versions for python packages are now injected from the latest git tag detected in the repo history, rather than from an otherwise-useless package.json. This is dependent on the project - for packages used in multiple projects, you have to decide what project you're building for. That's done right now through an environment variable. Happily enough, there's only two major projects that build python packages and they use different distribution formats and thus different makefile recipes, and thus can have different defaults. This also requires modifying a lot of the internals of how we do versions and while I was at it I went back to using phony targets in makefiles for wheels and sdists because yeah it means you have to rebuild them but it also makes the makefile way way way faster to parse. The now-unused package.jsons are removed, and since yarn now doesn't need to know about the python projects and they don't have package.json anymore they're pulled out of the workspace list. Sadly enough, this change means that any workflow that needs to produce versioned artifacts is going to need git history.
The versions in package.json are now all 0.0.0-dev. Versions do not yet come from git, but when they do they'll be injected into package bundles by webpack. That means that the yarn-level peer dependencies to other monorepo packages can't have explicit version requests in them - the local packages at 0.0.0-dev can't fulfill 6.1.0 or whatever, and we still have some old published packages on npm that will take priority. So we specify symlink dependencies. This ends up with the same structure on disk in node_modules as we used to have. The new structure showed some problems in a couple different tests that we fixed along the way. This commit just removes the version from package.json - it doesn't actually replace it with anything - so all js builds in this commit are now 0.0.0-dev unconditionally.
Javascript apps that injected their versions into their distribution bundles based on package.json now do it from git. This is based on a refactor and generalization of the version-from-git machinery that backs up the changelogs. Only some of our js apps are actually aware of their own version and make the version accessible inside the app. These are app, app-shell, discovery-client, labware-library, and protocol-designer. They all do it by using a webpack DefinePlugin to either override node.process.env to provide fake environment variables or establish globals that exist within the webpack bundle. That happens in webpack.config.js for those apps, so webpack.config.js now pulls the version that it injects from git. Since dev builds are webpacked, this is generally works there too. The exception is the desktop app. The desktop app uses electron-updater to do automatic updates. electron-updater's understanding of the current version is based on the electron App module's getVersion function, which pulls the version from the package.json and can't be overridden for some reason. In actual app builds (from make -C app-shell dist*), this is fine because we can selectively override package.json fields with configuration in electron-builder.config.js. When running the app in dev mode, we just run `yarn electron`, which will always try and pick up the version from app-shell package.json, which is 0.0.0-dev. That means that when you do a dev build of the app, the version in the settings tab will be correct (that's from webpack since it's in our js) but the updater will think it's the old version and you'll always get an update popup. We can live with that. Sadly enough, the above all means that github workflows that build versioned artifacts need to be cloned with fetch-depth 0 so they actually get history and tags.
Codecov Report
@@ Coverage Diff @@
## edge #11863 +/- ##
==========================================
+ Coverage 74.25% 74.34% +0.08%
==========================================
Files 2125 2150 +25
Lines 58877 59415 +538
Branches 6233 6238 +5
==========================================
+ Hits 43720 44171 +451
- Misses 13686 13773 +87
Partials 1471 1471
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
||
module.exports = { | ||
module.exports = async () => ({ | ||
appId: 'com.opentrons.app', | ||
electronVersion: '21.3.1', |
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.
maybe
const pkg = require("./package.json");
const electronVersion = pkg.devDependencies["electron"]);
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 think I don't want to do this because it would limit the version constraint in package.json to whatever electron-builder.config.js can accept (exact-version constraint) and also to the semantics we want. @mcous @b-cooper @shlokamin do yall know why we didn't single-source the electron version before?
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.
Prior to #11537, electron
was not listed in app-shell/package.json
, and was instead, a development dependency in the root package.json
. Keeping them separate was more of a philosophical thing than a technical thing, though we have had issues with electron-builder
selecting improper dependency versions for inclusion in the asar
.
I'm down to make this change, but let's give it its own PR. From reading the electron-builder
docs, I believe omitting the field is the suggested way of using the installed version from package.json
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.
Looks really good! Did not see any obvious problems with the code, but offered some readability suggestions. I will run some sanity checks for local builds but otherwise figure we'll rely pretty heavily on CI to tell us if this is working properly
api/src/opentrons/__init__.py
Outdated
HERE = os.path.abspath(os.path.dirname(__file__)) | ||
from ._version import version # noqa: E402 |
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.
Can this move above HERE
and ditch the E402 now that this file is more simple?
api/src/opentrons/__init__.py
Outdated
__version__ = package_json.get("version") | ||
except (FileNotFoundError, OSError): | ||
__version__ = "unknown" | ||
__version__ = version | ||
|
||
from opentrons import config # noqa: E402 |
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.
Same question as above
api/src/opentrons/_version.py
Outdated
_pyversion = sys.version_info[0:2] | ||
if _pyversion < (3, 7): | ||
raise RuntimeError( | ||
"opentrons requires Python 3.7 or above, this is {0}.{1}".format( | ||
_pyversion[0], _pyversion[1] | ||
) | ||
) |
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.
Not sure how helpful this is; down to avoid preserving it in this new system
|
||
module.exports = { | ||
module.exports = async () => ({ | ||
appId: 'com.opentrons.app', | ||
electronVersion: '21.3.1', |
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.
Prior to #11537, electron
was not listed in app-shell/package.json
, and was instead, a development dependency in the root package.json
. Keeping them separate was more of a philosophical thing than a technical thing, though we have had issues with electron-builder
selecting improper dependency versions for inclusion in the asar
.
I'm down to make this change, but let's give it its own PR. From reading the electron-builder
docs, I believe omitting the field is the suggested way of using the installed version from package.json
// `on` is used to hook into various events Cypress emits | ||
// `config` is the resolved Cypress config | ||
on( | ||
'file:preprocessor', | ||
webpackPreprocessor({ webpackOptions: require('../../webpack.config') }) | ||
webpackPreprocessor({ | ||
webpackOptions: await require('../../webpack.config')(), |
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.
Can we split up the require and the config call just to make it a little more clear what's going on here?
const webpackPreprocessor = require('@cypress/webpack-preprocessor')
const createWebpackConfig = require('../../webpack.config')
module.exports = async (on, config) => {
const webpackOptions = await createWebpackConfig()
on('file:preprocessor', webpackPreprocessor({ webpackOptions }))
}
scripts/git-version.js
Outdated
if (project === 'robot-stack') { | ||
return 'v' | ||
} else { | ||
return project + '@' |
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.
Same comment as above about template strings
scripts/git-version.js
Outdated
module.exports = { | ||
detailsFromTag: detailsFromTag, | ||
tagFromDetails: tagFromDetails, | ||
prefixForProject: prefixForProject, | ||
latestTagForProject: latestTagForProject, | ||
versionForProject: versionForProject, | ||
monorepoGit: monorepoGit, | ||
} |
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.
Can omit all : value
if variable name matches desired key
module.exports = { | |
detailsFromTag: detailsFromTag, | |
tagFromDetails: tagFromDetails, | |
prefixForProject: prefixForProject, | |
latestTagForProject: latestTagForProject, | |
versionForProject: versionForProject, | |
monorepoGit: monorepoGit, | |
} | |
module.exports = { | |
detailsFromTag, | |
tagFromDetails, | |
prefixForProject, | |
latestTagForProject, | |
versionForProject, | |
monorepoGit, | |
} |
__version__ = package_json.get("version") | ||
except (FileNotFoundError, OSError): | ||
__version__ = "unknown" | ||
from ._version import version # noqa: E402 |
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.
Same question about the noqa
_pyversion = sys.version_info[0:2] | ||
if _pyversion < (3, 7): | ||
raise RuntimeError( | ||
"opentrons requires Python 3.7 or above, this is {0}.{1}".format( | ||
_pyversion[0], _pyversion[1] | ||
) | ||
) |
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.
Same comment about not moving this forward to avoid side effects on import
* origin/edge: fix(api): ot3controller should utilize `check_ready_for_movement` (#11752) chore(docs): updating Google Tag Manager on docs subdomain #11855 refactor(app): add gripper attach, detach, and calibrate user-flow logic and skeletons (#11826) chore(build): fix push targets in api and s-d (#11879) fix(app): moveToLocation should be moveToMaintenancePosition (#11878) refactor(api): implement new Deck dependencies (#11853) refactor(api): implement new Deck collaborations (#11844) feat(app): create modals and add steps for 96 channel attach and detach (#11815) chore(build): Derive application versions from git (#11863) refactor(api): add support for Well.has_tip to the engine core (#11870) refactor(app, shared-data): calibration/moveToLocation to calibration/moveToMaintenancePosition (#11849) test(engine): add missing module_view test (#11872)
Overview
Until this PR, the way we did versioning was:
package.json
, which is something that's used in the js ecosystem. The python packages also had this.lerna
(which we wrapped in a makefile script) that lets you change the version in all those package.json files at the same time, as well as all the entries in js project package.jsons that set co-versioned dependencies on other projectsThis has a couple problems that hurt the way we do things:
One way to fix this (there are others) is to pull the source of truth for the "version" of something out of the content of the git repo and into the metadata of the git repo - as git tags. We already tag versions of different applications, but these tags are only descriptive - regardless of the state of the tag, a build of a given commit will always have the same version.
Instead, we can make the tag determine the version, when you build an application. We can do this by hooking into git repo metadata during the build process of each package - the versions injected into js applications by webpack can come from git tags, the versions injected into python packages by setup.py can come from git tags. This lets us do a couple very useful things.
The first new capability, and really the point of this PR, is that all packages are now capable of being in different applications that have different versions. The
opentrons
python package inapi/
can be built such that it uses the latest robot stack version (the latest tag starting withv
), and it will be when we build the OT-2 software - but it could also be built such that it uses the latest ot3 development version (the latest tag starting withot3
), and will be when we build the OT-3 software. If we wanted to stop coversioning the desktop app and robot software, it could even be built with the latest app version (call it the latest tag starting withapp@
) when it's intended for use in offline analysis. It could even be built with its own special version for deploying to pypi by using a different tag. This allows us to set up a separate release stream for the OT-3, with its own alpha/beta/release channels, so that we can issue releases without bothering customers.The second new capability, and maybe it's even better, is that you don't need a commit to change the version - you only need a tag. Think about the release process for the robot stack: that means there are no more bump PRs, each of which required two robot stack builds - the PR content, then the release branch after the merged PR - before you push a release tag.
The third new capability is a capability of understanding: all of our stacks are now versioned in the same way. Protocol designer and robot stack now work exactly the same with different tag prefixes. You can trigger anything you want on tags, you can push tags, the versions are separate, it's all the same. A commit of
components
can be atv6.7.8
as part of a robot stack release and at[email protected]
as part of a protocol designer release and at[email protected]
as part of a labware library release. There's no more conflict, no more little snippets in webpack config that dodge the way everything's configured by default - the process is all the same.This PR should both give us new build system capabilities and lessen the toil of making a robot stack release.
Changelog and Review Requests
This is a very broad PR that touches quite a few different files. It's organized into roughly standalone commits, and that's the way I'd recommend approaching it - JS folks, please take a look at the JS parts; python folks, please take a look at the python parts (or anybody look at anything, but if there's some part you're not interested in you can skip it pretty easily).
The commits are these:
Aspirationally update releasing.md
This commit updates the releasing docs to match the new workflow. It happened before the rest of the work, and is a good way to think about the goals of the rest of the work. Not much to look at here in particular, but copyedits always appreciated.
some changelogs crept back in
Another small one, fixes a merge issue from 6.2.0 - a previous pr removed the checked-in CHANGELOGS.md, and then the release merge brought them back, and this gets rid of them again.
do not use lerna for versions
The first step of the actual work: remove
lerna
as a dependency and removemake bump
. Leaves the contents of the rest of thepackage.json
s untouched, but after the following commits we won't need this anymore.find py versions from git
The first major commit in the PR. This
python_build_utils
to be able to get a version from gitpython_build_utils
to provide the versions for packagespython_build_utils
Reviewers here, please check out
remove versions from js package json
The first of the two commits that implement the above for js projects. It got split up because we needed more fixes for the js side. The js intra-monorepo dependencies were coversioned with the latest release; this won't work when the versions are all dynamically determined, and it won't work to make all those versions
0.0.0-dev
because we've previously released some packages to npm and yarn really wants to prioritize them. This means we now use yarn path dependencies with alink:
prefix which produces the samenode_modules
structure as before.Reviewers please leave some opinions on whether this is fine going forward or if we have to find another way
find js app versions from git
The second of the two commits that implement git based versioning for js projects. Here, we're now building in the actual versions to the bundled packages.
One thing to note is that right now, those versions are only present in the webpack output bundles. That means they won't be present in tests, lint, or mock.
In general these changes take their cues from how versions were injected into the web stack projects with webpack
DefineVersion
plugins - we just need to get the payload of those plugins from git instead of package.json. This works for all bundles, so dev servers as well as actual release bundles. The one odd-one-out here is of course the app.For the app, you can set the version in webpack config and the settings page will happily display the right thing... but you'll still get update popups as if the app was version 0.0.0-dev. The electron-updater pulls the "current version" of the app from electron.app.getVersion which is hardcoded to pull its data from the package.json of the package that bundles electron (in our case, app-shell). We solve this by injecting the version in the electron-builder config, but of course that only works when you're using electron-builder - when you make distributable build of the app (including those on CI, happily). When you run
make -C app dev
, we runyarn electron
inapp-shell
, and it pulls directly from package.json, and dev builds will therefore always think they're version 0.0.0-dev for the purposes of update, though the settings will show that they're the correct version.Reviewers, please let me know if I'm doing something... suboptimal... in any of these webpack plugins. I'd like to keep changes minimal here, but if there's something that hurts to look at please let me know.
Risk assessment
Medium. This changes the fundamentals of how we version things, but that's really not a problem until we do a new edge-sourced release, and the next couple of those we'll probably do are for internal distribution anyway so it's ok.
Closes RCORE-429