Skip to content
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

Document MSRV policy #881

Merged
merged 10 commits into from
Aug 6, 2023
Merged

Document MSRV policy #881

merged 10 commits into from
Aug 6, 2023

Conversation

wks
Copy link
Collaborator

@wks wks commented Aug 2, 2023

No description provided.

@wks wks requested a review from qinsoon August 2, 2023 06:30
README.md Outdated
## Supported Rust versions

As a research project, MMTk is quite aggressive in following the latest releases of the Rust
toolchain. The minimum supported Rust version (MSRV) is "N-1", that is, one minor version before the
Copy link
Member

@qinsoon qinsoon Aug 2, 2023

Choose a reason for hiding this comment

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

I don't feel we want to eagerly increase our MSRV. Having a low MSRV is a benefit for users, but a drawback for our development. We can have something like "N-1", which means we can increase our MSRV to that version if there is a reason for us to do so (e.g. dependencies, new language features, etc), but we guarantee that the MSRV will be at least "N-1". In this case, we do not have to eagerly increase MSRV for no reason (which bothers users), and we can increase MSRV in the situation like what we are experiencing right now, according to the policy.

This is just my thought.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that's also OK. My understanding is,

  1. We always guarantee that "N-1" is supported.
  2. But the actual rust-version field in Cargo.toml may sometimes be older than N-1 if we didn't make use of any newer features during the last 6 weeks of development, and no dependencies require a newer rust-version.
  3. If, during our development, we made a PR that uses a newer feature introduced in N-m where M>=1, we have a good reason to bump rust-version to N-m.
  4. The CI shall test with both the latest stable version and the rust-version in Cargo.toml.

That still sounds good, but I still prefer bumping the MSRV eagerly (even for "no reason"), for the following reasons:

  1. If the MSRV is already bumped after a release, we don't have to deliberately bump it during development.
  2. If we cannot guarantee that N-2 is supported, our users still cannot rely on N-2 even if the rust-version in Cargo.toml is sometimes N-2 or older. That's because it may change in the next release. Although the user may lock the version of mmtk-core to a version to satisfy their MSRV requirement, we should not encouraged this (at least for now) because MMTk is being rapidly developed, and relying on an outdated MMTk now will make it difficult for the users to migrate to the latest MMTk in the future.

However, we may change the policy if MMTk reached a stable state. By that time, we may prefer letting the MSRV "linger" at a version, like you suggested, if we do not make use of newer features. That will allow the users to use a newer mmtk-core while still using an older Rust toolchain. At that time, it should be reasonable for the user to lock the version of mmtk-core because mmtk-core is relatively stable. But I don't think MMTk has reached such a state.

README.md Outdated Show resolved Hide resolved
README.md Outdated
latest stable Rust release. For example, if the latest Rust stable release is "1.61.2", then the
MSRV will be "1.60.0".

MMTk has a six-week release cycle, roughly the same as Rust itself. We bump the MSRV version
Copy link
Member

Choose a reason for hiding this comment

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

See my comment above. Also it does not mention the Rust version we use in rust-toolchain. I feel it implies the version in rust-toolchain will be the latest.

Last time we talked with @caizixian on this. We agreed on bumping the version in rust-toolchain for every other release (he needs to update our dev and CI machines).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Last time we talked with @caizixian on this. We agreed on bumping the version in rust-toolchain for every other release (he needs to update our dev and CI machines).

If updating the toolchains every 6 weeks is too much work, we can just say our MSRV is "N-2". I am still OK with "N-2" or "N-3". In the case of "N-2", MMTk-core will support three Rust stable versions, and each Rust version remain supported for 18 weeks. If we update our toolchains for every even (or odd) Rust releases (that is, every 12 weeks), the existing installed toolchain will remain supported by the time we do our update. In other word, on the day when an even Rust version (assume we update Rust at even versions) is released, both the currently installed toolchain and the newest toolchain will be supported, and we can do the update in the window of 6 weeks from that day until the next Rust release. (Actually we may have up to 12 weeks because the MSRV of MMTk core is the "N-2" right after the previous MMTk core release.)

@caizixian What do you think of it? If "N-2" is still too much work, we can use "N-3" instead.

Copy link
Member

Choose a reason for hiding this comment

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

There is some confusion here. We have two Rust versions recorded in the repo: the version we use for testing and development (toolchain version) in the rust-toolchain file, and the minimal Rust version we support (MSRV) in Cargo.toml. When you say N-2, N is the toolchain version I assume, and the N-2 is MSRV.

When we bump Rust toolchain version, we need Zixian to update our dev machines. But for MSRV, we do not need any work from Zixian (in the current setup, we do not even need the MSRV installed on our dev machines -- we use Github runners to verify we can build with the version).

So using N-2 or N-3 doesn't really matter for Zixian. But how often we bump our toolchain version matters. I was saying the description added here implies we bump our toolchain version for every release.

@caizixian
Copy link
Member

I think @qinsoon is right that there are two separate problems here. The rust-toolchain in Cargo.toml is MSRV, while the rust-toolchain file tells rustup which toolchain to use.

A conservative MSRV helps downstream projects that depend on MMTk, so that they are not forced to update their toolchain unless we decide that newer language features are worth bumping MSRV.

Separately, pinning the Rust toolchain picked up by rustup (which is always >= MSRV) is important for reproducibility. Bumping that can help us access newer compiler checks and optimization (we will not accidentally depend on newer language feature as long as we have MSRV CI check in place). However, bumping that too frequency increases work on the infrastructure side (we need to make sure all machines we use actually have that specific version of the toolchain installed rather than just some random version >= MSRV), and it can also muddle the performance regression results because it will be harder to tease out performance trends of MMTk from the changes of the underlying toolchains.

@wks
Copy link
Collaborator Author

wks commented Aug 3, 2023

Sorry. I completely forgot the rust-toolchain file.

Actually I doubt about the effectiveness of rust-toolchain file. I think we should always test (for correctness) against the latest stable release of Rust. It happened once that we had a UB in the latest stable version, but since rust-toolchain is a much older version, we didn't discover it until I tried to run some code in the Ruby binding using the latest release. Currently, our CI is testing 1.61.0 (MSRV) and 1.66.1 (rust-toolchain), but the latest version is 1.71.0.

One positive side of the rust-toolchain file, however, is that it records the "latest version with which mmtk-core is known to compile". I found it useful when I tried to compile a legacy Rust program last released about 3 years go which depends on something like Rust 1.45.0. But this should not happen for mmtk-core. In our current stage, if mmtk-core doesn't compile with the latest Rust toolchain, it is a serious problem. We may use rust-toolchain to temporarily fix the toolchain version so that some of us can fix the compilation error while others can move forward developing other parts of mmtk-core using the "last known working version" of Rust. But I think it is not good to lock the Rust version using rust-toolchain in a steady state.

@caizixian For "reproducibility", I think the answer is that whenever we do performance test, we always record the build string (including the exact version of Rust toolchain) that we used for building. (see this issue and this PR) This can be the version of Rust toolchain actually installed on the moma machines at the time of doing the test, instead of rust-toolchain file in the repo. For performance history plots like this one, we should also mark the points when the toolchain of a data point is different from the previous one. (That point will usually be the day when we update the images.)

I suggest we do the following

  1. Remove rust-toolchain so that we always develop using the latest stable version, unless mmtk-core failed to compile on that version.
  2. Decide the period at which we update the images of the moma machines. Let's say we update the image for every X Rust releases.
  3. Decide the m in the N-m MSRV policy. m should be set so that N-m to N covers a slightly longer period of time than X. This will give us a time window for us to update the moma machines. It should be OK if we let m = X.
  4. We bump MSRV after every MMTk-core release.

image

If m == X, in the worst case we will have a grace period of six weeks to update the moma machines before the MSRV of the master branch goes beyond the installed version on moma machines.

But I still remember @steveblackburn has some opinion about the rust-toolchain file, too. IIRC it is about everyone using the same Rust version. If we really have to have rust-toolchain, I suggest it should simply contain whatever version we have installed on the moma machines, and bump it when we update the moma machines. We adjust MSRV (the package.rust-version field in Cargo.toml) independently according to the method I described above.

@qinsoon
Copy link
Member

qinsoon commented Aug 3, 2023

If we really have to have rust-toolchain, I suggest it should simply contain whatever version we have installed on the moma machines, and bump it when we update the moma machines. We adjust MSRV (the package.rust-version field in Cargo.toml) independently according to the method I described above.

This is roughly what we are doing. We update moma machines before a MMTk release, update the rust-toolchain version we use in tests, and make changes about whatever is needed for the new Rust version.

@qinsoon
Copy link
Member

qinsoon commented Aug 4, 2023

We discussed this:

  • We will add another test to check if MMTk core can build with the latest Rust stable. The test may fail, it does not prevent merging PRs.
  • We can update our code base to make it work with the latest Rust, but we do not update the Rust version in the rust-toolchain file.
  • We update the rust-toolchain file before a release, and coordinate this with Zixian on moma machines.

One thing we haven't discussed:

  • Do we eagerly increase MSRV or lazily? I still suggest doing that lazily, just for the sake of our users.

@wks
Copy link
Collaborator Author

wks commented Aug 4, 2023

* We update the `rust-toolchain` file before a release, and coordinate this with Zixian on moma machines.

I think you mean updating rust-toolchain to a version already installed on moma machines, but not beyond that. If the latest version installed on moma machines is 1.71.0, we update rust-toolchain to 1.71.0 before our next release. If, for example, Zixian updates the moma machines in October so that the latest stable on moma machines become 1.73.0, then in our release in November we will update rust-toolchain to 1.73.0.

* Do we eagerly increase MSRV or lazily? I still suggest doing that lazily, just for the sake of our users.

I think it is good enough to increase MSRV on demand. Well, in practice, we always need a PR to bump MSRV after a release. It's just a matter whether we do it in a dedicated PR, or piggy-back the change with a PR that uses the new feature.

@qinsoon
Copy link
Member

qinsoon commented Aug 4, 2023

* We update the `rust-toolchain` file before a release, and coordinate this with Zixian on moma machines.

I think you mean updating rust-toolchain to a version already installed on moma machines, but not beyond that. If the latest version installed on moma machines is 1.71.0, we update rust-toolchain to 1.71.0 before our next release. If, for example, Zixian updates the moma machines in October so that the latest stable on moma machines become 1.73.0, then in our release in November we will update rust-toolchain to 1.73.0.

Yeah. We update to the latest Rust stable before a release, and coordiante with Zixian to install that version. We do not merge it unless the version is installed on the moma machines and pass all the tests (including binding tests).

* Do we eagerly increase MSRV or lazily? I still suggest doing that lazily, just for the sake of our users.

I think it is good enough to increase MSRV on demand. Well, in practice, we always need a PR to bump MSRV after a release. It's just a matter whether we do it in a dedicated PR, or piggy-back the change with a PR that uses the new feature.

If we increase MSRV on demand, that means we don't have to bump MSRV for a release. We bump it when we need to use new features, new dependencies, or other reasons. The new policy allows us to bump MSRV to any version that is "N-m" (N is the version in the rust-toolchain file).

@wks
Copy link
Collaborator Author

wks commented Aug 4, 2023

... We do not merge it unless the version is installed on the moma machines and pass all the tests (including binding tests).

As long as it passes tests before merging, it should be OK.

If we increase MSRV on demand, that means we don't have to bump MSRV for a release. We bump it when we need to use new features, new dependencies, or other reasons. The new policy allows us to bump MSRV to any version that is "N-m" (N is the version in the rust-toolchain file).

Yes. We can still write in the document that 'we support "N-m" MSRV policy' (after discussion with @caizixian, m can be 1 because the image-updating process is automated, and we can update image every month) so that we know we already have a policy if we want to bump MSRV, and other people know we may bump our MSRV to that range in the next release.

@wks
Copy link
Collaborator Author

wks commented Aug 4, 2023

I have just updated the contents of README.md to reflect the results of our discussions.

qinsoon added a commit that referenced this pull request Aug 4, 2023
This PR adds checks to build and unit test MMTk core with the latest
Rust stable version. We do not require the tests with latest stable Rust
to pass in order to merge a PR. See discussion in
#881 (comment).
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
wks and others added 4 commits August 6, 2023 15:26
Co-authored-by: Yi Lin <qinsoon@gmail.com>
Co-authored-by: Yi Lin <qinsoon@gmail.com>
Co-authored-by: Yi Lin <qinsoon@gmail.com>
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@caizixian caizixian left a comment

Choose a reason for hiding this comment

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

LGTM. I added some further clarifications and you can adopt them where you see fit.

caizixian and others added 3 commits August 6, 2023 18:15
Co-authored-by: Zixian Cai <2891235+caizixian@users.noreply.github.com>
Co-authored-by: Zixian Cai <2891235+caizixian@users.noreply.github.com>
@wks wks merged commit 3740deb into mmtk:master Aug 6, 2023
14 of 17 checks passed
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