Skip to content

Conversation

@kelset
Copy link
Contributor

@kelset kelset commented Sep 29, 2022

Summary

While working on #34513 I noticed that on main branch the versioning is not really consistent everywhere. So this PR is an attempt at realigning so that on the main branch, RN is 1000.0.0 everywhere - in a way, it's cleaning up the room for the monorepo work to go flawlessly).

It's just a pass of node scripts/set-rn-version.js --to-version 1000.0.0.

There's the small chance that some versions where kept to 0.0.0 on purpose (build tools are weird), so we might just have to close this off. No big deal :)

Changelog

[Internal] [Changed] - re-align version to be 1000.0.0 on main everywhere

Test Plan

CI is green and when imported, nothing breaks.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Microsoft Partner: Microsoft Partner labels Sep 29, 2022
@kelset kelset requested a review from cortinico September 29, 2022 11:24
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Sep 29, 2022
@analysis-bot
Copy link

analysis-bot commented Sep 29, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 3037190
Branch: main

@kelset kelset added the Tech: Monorepo For PRs that are related to the monorepo infra label Sep 29, 2022
@facebook-github-bot
Copy link
Contributor

@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.


exports.version = {
major: 0,
major: 1000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Change looks fine to me. Let's just make sure that the nightly and the commitly jobs are effectively changing this to 0. I'd like to avoid to publish 1000.0.0-20220928-2025-527cc3426 on NPM just because the script it's appending the date/sha 👍

Copy link
Contributor Author

@kelset kelset Sep 29, 2022

Choose a reason for hiding this comment

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

we should be fine for nightlies, the flag sets the version to 0.0.0 here:

const rawVersion =

and then releaseversion gets modified here:

releaseVersion = `${version}-${dateIdentifier}-${shortCommit}`;

then according to this

// For stable, pre-release releases, we rely on CircleCI job `prepare_package_for_release` to handle this

we can see this

node ./scripts/prepare-package-for-release.js -v << parameters.version >> -l << parameters.latest >>

and finally here we have
https://github.com/facebook/react-native/blob/main/scripts/prepare-package-for-release.js#L57

so we should be good to go, both are using the same set-rn-version so they'll touch the same files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but that said, for commitlies it's not setting it to 0.0.0 - uh.

We might need to change this

nightlyBuild

to use isCommitly? 🤔
Why are commitlies not set to 0.0.0? can they be happening on stable branches?
cc @lunaleaps ?

Copy link
Contributor

@lunaleaps lunaleaps Sep 29, 2022

Choose a reason for hiding this comment

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

Hmm trying to page this all back in. Yes I believe commitlies can happen on stable branches -- I think the idea was that we could have PRs against the stable branch if people wanted to release a new patch -- and could use the commitly to verify but that may not actually be a workflow we need to support.

I think the 0.0.0 versioning is just that we could publish nightlies and it wouldn't affect npm version ordering but I think commitlies probably were grandfathered in to keep the 1000 version (I don't know why this was chosen)-- I don't think it was an intentional thing. I think we could probably update everything to 0.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, I'm stupid - I should have read the comment at the start of the file:

* * Version the commitly of the form `1000.0.0-<commitSha>`

looks like currently we do have commitlies in the 1000.0.0 format - @cortinico I don't know if we want to move away from the pre-established approach.

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 currently we do have commitlies in the 1000.0.0 format - @cortinico I don't know if we want to move away from the pre-established approach.

Ideally yes. I would refrain from packaging anythign on 1000.x versioning scheme.
Commitlies are just nightlies from my point of view (with a different date/time and sha) + we don't npm publish them. Aside from this, versioning schema should be the same.

As @lunaleaps, commitlies are avaialble on every commit, also release branches. In an ideal world, release managers should test the commitly before triggering the publish job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok so just to make sure I'm understanding - I should change the logic here to tag/version commitlies to use 0.0.0, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't necessarily need to happen here. Can be a separate PR like "re-align commitlies versioning schema to follow the same as nightlies" or so 👍

@kelset kelset force-pushed the kelset/set-main-version-to-1000 branch from 22abff3 to a1e8457 Compare September 30, 2022 09:43
@analysis-bot
Copy link

analysis-bot commented Sep 30, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,739,305 +13
android hermes armeabi-v7a 7,141,962 +27
android hermes x86 8,050,386 +5
android hermes x86_64 8,022,231 +34
android jsc arm64-v8a 9,607,415 -64
android jsc armeabi-v7a 8,372,903 -65
android jsc x86 9,555,086 -55
android jsc x86_64 10,147,662 -63

Base commit: 32b6f31
Branch: main

@cortinico
Copy link
Contributor

@kelset a rebase should make the CI green 👍

@kelset kelset force-pushed the kelset/set-main-version-to-1000 branch from a1e8457 to a7f2759 Compare October 3, 2022 14:00
@kelset
Copy link
Contributor Author

kelset commented Oct 3, 2022

@cortinico rebased 👍 🤞

@facebook-github-bot
Copy link
Contributor

@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @kelset in 232e447.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Oct 4, 2022
@kelset kelset deleted the kelset/set-main-version-to-1000 branch October 4, 2022 16:34
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…4817)

Summary:
While working on facebook#34513 I noticed that on main branch the versioning is not really consistent everywhere. So this PR is an attempt at realigning so that on the main branch, RN is 1000.0.0 everywhere - in a way, it's cleaning up the room for the monorepo work to go flawlessly).

It's just a pass of `node scripts/set-rn-version.js --to-version 1000.0.0`.

There's the small chance that some versions where kept to 0.0.0 on purpose (build tools are weird), so we might just have to close this off. No big deal :)

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[Internal] [Changed] -  re-align version to be 1000.0.0 on main everywhere

Pull Request resolved: facebook#34817

Test Plan: CI is green and when imported, nothing breaks.

Reviewed By: cortinico

Differential Revision: D39926953

Pulled By: cortinico

fbshipit-source-id: ff66530382f891e17c00b35edf97c03591b6a9a8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Tech: Monorepo For PRs that are related to the monorepo infra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants