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

msrv: Bump to Rust 1.48 #2381

Merged
merged 1 commit into from
Dec 15, 2020
Merged

Conversation

cgwalters
Copy link
Member

@cgwalters cgwalters commented Dec 11, 2020

We need this for https://cxx.rs

While we're here:

  • Add some more comments/links
  • Since the Rust bits are now at the toplevel, we can explicitly
    invoke cargo
  • And since we can do that, use the + syntax to specify the
    toolchain explicitly

@jlebon
Copy link
Member

jlebon commented Dec 14, 2020

Note 8.3 is still on 1.45. We recently did a rebase there because of library versioning issues when cherry-picking from 8.4. Let's hold this until 1.47 gets into 8.3? Otherwise, I think it restricts us to backports only until RHCOS moves to 8.4, which is still a long while away.

@cgwalters
Copy link
Member Author

I guess we can carry a fork of cxx-rs for now.

@cgwalters
Copy link
Member Author

Two counters though:

  • The chance that we'll need to backport something else to 8.3 seems somewhat low
  • If we do need to backport, we can actually create a rhel-8-3 branch upstream and cherry pick patches there. RHEL culture tends to prefer cherry picking patches versus wholesale upstream bumps anyways.

@cgwalters
Copy link
Member Author

And third, I am pretty sure nothing stops us from explicitly requesting a newer toolset even for 8.3 buildroots.

(Aside; now that I look it seems like cxx.rs emits a warning unless it's built with ≥1.48 so I'll bump this PR)

@cgwalters
Copy link
Member Author

For an upstream git branch we could call it e.g. stable-2020-1 and say we'll support it a while? Or explicitly call it openshift-4.6 or so.

@cgwalters cgwalters changed the title msrv: Bump to Rust 1.47.0 msrv: Bump to Rust 1.48 Dec 14, 2020
@jlebon
Copy link
Member

jlebon commented Dec 14, 2020

Yeah, I'm not too concerned about backporting patches. Just talking about regular periodic rebases. Let's go over the dates internally.

And third, I am pretty sure nothing stops us from explicitly requesting a newer toolset even for 8.3 buildroots.

If this is true, then that helps I think.

We need this for https://cxx.rs

While we're here:

 - Add some more comments/links
 - Since the Rust bits are now at the toplevel, we can explicitly
   invoke `cargo`
 - And since we can do that, use the `+` syntax to specify the
   toolchain explicitly
@cgwalters
Copy link
Member Author

Let's go over the dates internally.

Actually a recent major RHEL change is that the dates are predictable and documented:

https://access.redhat.com/support/policy/updates/errata/#RHEL8_Life_Cycle

From that we can see that 8.4 is ~6 months away 😢

@cgwalters
Copy link
Member Author

OK so there are various options:

  1. branch: Branch and backport patches if necessary (my preference I think)
  2. tag: Check about tagging in newer toolset in a side tag (probably not hard but requires various procedural stuff)
  3. fork: Try to fork cxx.rs instead and make it build with 1.45; I looked at this and it would be annoying as there's a lot of const fn stuff and it also makes it much harder to take advantage of newer cxx.rs features and contribute upstream
  4. wait: Don't try to land cxx.rs stuff for...a while (doesn't have to be 6 months but maybe until it becomes clear we're not rebasing in 8.3 again)

The argument for branching also include:

  • RHEL culture prefers backporting patches anyways versus rebasing
  • The cxx.rs effort will also involve a fair bit of code churn and that brings in regression risk that also argues for branching
  • Nothing is blocking us from landing updates in 8.4! Just 8.3.

@cuviper
Copy link

cuviper commented Dec 14, 2020

And third, I am pretty sure nothing stops us from explicitly requesting a newer toolset even for 8.3 buildroots.

I would not want to disrupt Rust in the common 8.3 buildroots, especially since pulling back builds from 8.4 would need LLVM too. If necessary, we could do something in a side tag, but that should be an exceptional situation.

@cuviper
Copy link

cuviper commented Dec 14, 2020

3. fork: Try to fork cxx.rs instead and make it build with 1.45; I looked at this and it would be annoying as there's a lot of const fn stuff and it also makes it much harder to take advantage of newer cxx.rs features and contribute upstream

3(alt): use cxx 0.5.10 while you're still targeting 8.3, as it only required 1.43. (also sacrificing new features though)

@cgwalters
Copy link
Member Author

3(alt): use cxx 0.5.10 while you're still targeting 8.3, as it only required 1.43. (also sacrificing new features though)

It's not just about features - cxx 1.0 is defined to be stable (plus there's a ton of bugfixes and improvements already).

@jlebon
Copy link
Member

jlebon commented Dec 14, 2020

3(alt): use cxx 0.5.10 while you're still targeting 8.3, as it only required 1.43. (also sacrificing new features though)

It's not just about features - cxx 1.0 is defined to be stable (plus there's a ton of bugfixes and improvements already).

Are many of those bugfixes relevant to us, or could we make do with 0.5.10 for now? Would we have to rewrite a ton of stuff once we're unblocked on that front? Could we branch cxx at 0.5.10 for now to carry the fixes we really need, if any?

@cuviper
Copy link

cuviper commented Dec 14, 2020

(Aside; now that I look it seems like cxx.rs emits a warning unless it's built with ≥1.48 so I'll bump this PR)

It looks like the thing that actually needs 1.48 is the syntax change allowing unsafe extern "C++", rust#75857 and cxx#456, but you only use extern "Rust" in #2336. We'll have 1.47 in RHEL 8.3.1, so perhaps that can hold you over?

The warning comes from cxx's own build script, which I believe Cargo will mask by default because it uses --cap-lints for dependencies. If cxx ever turns that into a hard error, you'd have to limit yourself to a version before it starts that.

@cgwalters
Copy link
Member Author

I think regardless we probably shouldn't just rebase 8.3.z on the latest upstream after we start work on this. Possibly not even 8.4.
We spend a lot of work maintaining both FCOS and RHCOS and while it usually helps keep us sane to keep things in lockstep where we can, I think we also should take advantage sometimes of the fact that we do maintain two distributions to let them diverge sometimes and have FCOS test out features first. Like this work.

@cgwalters
Copy link
Member Author

OK I tried downgrading to 0.5.10 and I'm getting a build error that I think is dtolnay/cxx#311
I'm really not seeing the cost/benefit of going for the "old cxx" model over branching.

@jlebon
Copy link
Member

jlebon commented Dec 14, 2020

I think regardless we probably shouldn't just rebase 8.3.z on the latest upstream after we start work on this. Possibly not even 8.4.
We spend a lot of work maintaining both FCOS and RHCOS and while it usually helps keep us sane to keep things in lockstep where we can, I think we also should take advantage sometimes of the fact that we do maintain two distributions to let them diverge sometimes and have FCOS test out features first. Like this work.

Sure, I think that's fine. If it weren't hard to leave that door open then I would've preferred doing that. It might come back to bite us if we have gnarly patches we need to backport, but we've done it before.

/approve

@jlebon
Copy link
Member

jlebon commented Dec 15, 2020

/lgtm

CI timed out; restarted it.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon, travier

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [cgwalters,jlebon,travier]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit b3b4dd3 into coreos:master Dec 15, 2020
@cgwalters
Copy link
Member Author

Any opinions on the branch naming? openshift-4.7-branch ? Or rhel-8-stable-branch?

@jlebon
Copy link
Member

jlebon commented Dec 16, 2020

Maybe just rhel-8.3? But meh, feels like we can wait to create it until we need it.

@jlebon jlebon mentioned this pull request Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants