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

Get rust toolchain to stable for all builds #58

Merged
merged 2 commits into from
May 5, 2023
Merged

Get rust toolchain to stable for all builds #58

merged 2 commits into from
May 5, 2023

Conversation

bbenne10
Copy link
Contributor

@bbenne10 bbenne10 commented Mar 7, 2023

I believe that this is the minimum required changeset to get CI builds passing with modern Rust toolchains.
Developers working on nsncd should use rustup (or some other method) to get a modern stable toochain (but I have not made documentation changes speaking to this).

No new artifacts are pushed.
No changes to the running of ci/test.sh have been made.

I have updated slog-term simply to test that the msrv is new enough to build the newer version.
Feel free to revert that change, if you so desire.

@bbenne10 bbenne10 mentioned this pull request Mar 7, 2023
@bbenne10
Copy link
Contributor Author

Went ahead and also updated Cargo.toml and Cargo.lock. I updated all the pinned dependencies to their latest releases. I have not updated the pins on any other dependencies.

@flokli
Copy link
Contributor

flokli commented Mar 11, 2023

@leifwalsh @geofft can you make a decision about this, either here or in #54?

Happy to rebase my PR once this has landed.

@leifwalsh
Copy link
Collaborator

I think this looks reasonable, but I need Geoff to weigh in, in terms of how we'd import/build it after merging this. He's very busy right now on some other internal work so I'm trying not to bother him too much. Hopefully some time in the next couple weeks we'll be able to make a decision.

@bbenne10
Copy link
Contributor Author

@leifwalsh: if you would like the Debian 10 deb package as an artifact, that would be trivial to add and then the possibility opens up of having an pre-built installable package from every merge to main.

Im happy to continue work on this (I selfishly want an RPM for RHEL and Deb for Ubuntu latest available as artifacts too, but that isnt a problem you all need to solve for us).

@bbenne10
Copy link
Contributor Author

Just checking in. Has any bandwidth freed up?

@leifwalsh
Copy link
Collaborator

My apologies, no, not yet. I promise we want to make this happen and keep collaborating with you all :)

@geofft or someone from his team will do the checks we need to some time in April, and I'll do my best to make sure we stick to that. Once we get this or something like it merged and have our internal stuff set up to deploy it, I think that unblocks all the other work the other folks are trying to upstream, or at least, I can review those without as much blocking on Geoff's team.

In the meantime, yes, I think getting debs and RPMs published as artifacts sounds ideal. I can probably take a look at that too.

@bbenne10
Copy link
Contributor Author

I will take a look at getting debs and rpms published for this at some point soon.

@leifwalsh
Copy link
Collaborator

Ok we're fine with deploying debs built in CI here. So it's just up to one of us to make that happen.

@leifwalsh
Copy link
Collaborator

Started getting a little bit somewhere #59

@leifwalsh
Copy link
Collaborator

Ok, publishing debs works, feel free to rebase this and make whatever additional change you wanted here.

Did we already ask you to sign and send in the CLA?

@bbenne10
Copy link
Contributor Author

bbenne10 commented Apr 17, 2023

I sent in a signed CLA before I started work on this. I got an Out of Office response from the receiver (I can find out who if necessary), but didn't follow up after that. I assumed that the CLA in someone's inbox was probably enough :P

I've gone ahead and simply rebased these changes forward. I'll have to get to using cargo plugins for rpm/deb building when I have a bit more free time. I will open a new PR for that work.

@bbenne10
Copy link
Contributor Author

I am unsure why the codecov report is failing to upload and I'm not entirely sure it's this PR's fault?

@bbenne10
Copy link
Contributor Author

Looking at the error again - I think this is either a problem with codecov's servers or with the api token that's stored as a secret in the config for this repo. I can fix neither. My hands are tied until someone can resolve this for me :/

@flokli
Copy link
Contributor

flokli commented Apr 25, 2023

@leifwalsh with #59 and #60 in, can you take a look at this? Maybe code coverage must be special-cased for external PRs?

@leifwalsh
Copy link
Collaborator

Yeah, sorry, we're fighting glibc compat stuff in #61. Once we get that sorted out I think we can merge this and take a look at the others again.

I don't think we care about coverage for these, it's probably just a codecov misconfiguration and we can deal with that separately.

@bbenne10
Copy link
Contributor Author

bbenne10 commented May 1, 2023

Looking over #61, it looks like you've basically made the same change I am making here in 3f8addc. Once that PR is merged, there's no point to this one any longer.

@leifwalsh
Copy link
Collaborator

@bbenne10 I think you can rebase this now, to just include the dependency version changes, and we can see how that goes.

@bbenne10
Copy link
Contributor Author

bbenne10 commented May 4, 2023

Done. Took the liberty of rewriting the history of this branch a bit too. Just squashed everything into a single commit that updates the dependency versions rather than keeping the cargo.lock updates in their own commit.

@leifwalsh leifwalsh self-requested a review May 5, 2023 01:57
@leifwalsh leifwalsh merged commit 125cb88 into twosigma:main May 5, 2023
@leifwalsh
Copy link
Collaborator

@bbenne10 thanks for sticking it out with us :)

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.

None yet

3 participants