Skip to content

Fix release script --commit param #20720

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 3, 2021
Merged

Fix release script --commit param #20720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions scripts/release/download-experimental-build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,13 @@ const {

const checkEnvironmentVariables = require('./shared-commands/check-environment-variables');
const downloadBuildArtifacts = require('./shared-commands/download-build-artifacts');
const getLatestMasterBuildNumber = require('./shared-commands/get-latest-master-build-number');
const parseParams = require('./shared-commands/parse-params');
const printSummary = require('./download-experimental-build-commands/print-summary');

const run = async () => {
try {
addDefaultParamValue('-r', '--releaseChannel', 'experimental');
addDefaultParamValue(
null,
'--build',
await getLatestMasterBuildNumber(true)
);
addDefaultParamValue(null, '--commit', 'master');
Copy link
Contributor

@bvaughn bvaughn Feb 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is a simpler solution but as it stands after this PR, the Firefox DevTools release process will be broken (because their testers need to have a reproducible from source and scripts/release/download-experimental-build.js --commit=master isn't a valid way to repro a specific build). I'll propose a small change with a follow up PR though!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm do we not include the build-info.json with CI builds anymore? I was hoping we could read from it instead but I don't see it in the build2 artifacts.

Copy link
Contributor

@bvaughn bvaughn Feb 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ooh now I see this:

react/scripts/release/publish-commands/print-follow-up-instructions.js

Lines 39 to 43 in 0e526bc

// FIXME: New build script does not output build-info.json. It's only used
// by this post-publish print job, and only for "latest" releases, so I've
// disabled it as a workaround so the publish script doesn't crash for
// "next" and "experimental" pre-releases.
const {commit} = readJsonSync(

But is there a reason why the new build script doesn't/can't output it? We're in a weird space now where our scripts both validate and require that build-info.json is in the package files array and we no longer publish it 😁

I guess I can always parse it out of the build number?

Copy link
Contributor

Choose a reason for hiding this comment

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

Some follow up: #20723

Copy link
Collaborator Author

@acdlite acdlite Feb 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

But is there a reason why the new build script doesn't/can't output it?

Figured it was worth seeing if it's actually necessary anymore. Might be nice to stop requiring every package to publish that file to npm, if we can.

I believe the last remaining reason we need it is because the prepare-release-from-npm needs it. But maybe we can instead pull the build artifacts from CI, like we do with prereleases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess, but at the same time I don't see much of a downside of publishing it. It adds a little complexity in one place (but we already have it in place and well "tested") in exchange for avoiding it in another (having to pull a duplicate artifact from CI somehow– which seems difficult to do if we don't have build-info)


const params = await parseParams();
params.cwd = join(__dirname, '..', '..');
Expand Down
8 changes: 2 additions & 6 deletions scripts/release/shared-commands/parse-params.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,8 @@ module.exports = async () => {
const params = commandLineArgs(paramDefinitions);

if (params.build !== null) {
if (params.commit !== null) {
console.error(
'`build` and `commmit` params are mutually exclusive. Choose one or the other.`'
);
process.exit(1);
}
// TODO: Should we just remove the `build` param? Seems like `commit` is a
// sufficient replacement.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the release script README is in a broken state at the moment. I'll take a pass at that too with an update PR.

} else {
if (params.commit === null) {
console.error('Must provide either `build` or `commit`.');
Expand Down