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

A few CI patches #2516

Merged
merged 3 commits into from
Feb 1, 2021
Merged

A few CI patches #2516

merged 3 commits into from
Feb 1, 2021

Conversation

cgwalters
Copy link
Member

ci: Drop testuser creation

Nothing is using this; our unit tests don't change uids and
most of our testing is in VMs.

Dropping this makes it easier to run the scripts outside of CI.


ci: Split clang into separate script, run it in CoreOS CI

Let's do a build with clang as a cleanly separate context
instead of serially; and also do it unconditionally. This
is prep for turning on more -Werror flow in both cases,
and also using clang scan-build in CI.


ci: Drop custom msrv checking

The way this tries to replace the system Rust is hacky and
actually I realized belatedly I may have broken it recently; basically
installdeps.sh re-adds the system one, and it's hard to be sure
with our current buildsystem we're using the newer one from $PATH.

What we really want to do here is use a CentOS8 buildroot,
which will automatically enforce this in a better way along
with solving other problems. But right now we've broken
that because libdnf requires a too-new libmodulemd.

So let's just rely on the Fedora rust for now.

Nothing is using this; our unit tests don't change uids and
most of our testing is in VMs.

Dropping this makes it easier to run the scripts outside of CI.
Let's do a build with clang as a cleanly separate context
instead of serially; and also do it unconditionally.  This
is prep for turning on more `-Werror` flow in both cases,
and also using clang `scan-build` in CI.
The way this tries to replace the system Rust is hacky and
actually I realized belatedly I may have broken it recently; basically
`installdeps.sh` re-adds the system one, and it's hard to be sure
with our current buildsystem we're using the newer one from `$PATH`.

What we really want to do here is use a CentOS8 buildroot,
which will automatically enforce this in a better way along
with solving other problems.  But right now we've broken
that because libdnf requires a too-new libmodulemd.

So let's just rely on the Fedora rust for now.
Copy link
Contributor

@lucab lucab left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, lucab

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:

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 e337562 into coreos:master Feb 1, 2021
@jlebon
Copy link
Member

jlebon commented Feb 24, 2021

ci: Drop custom msrv checking

This is unfortunate. I agree having a CentOS 8 buildroot here would be better, though until then I think we should keep the MSRV codified and verified and manually bumped. Even if we agree to run ahead of RHEL's Rust for a bit, if we don't do any tracking, then it makes planning future rebases much harder because there's no guarantee of what our true MSRV is and how it corresponds to RHEL.

Thoughts on adding it back?

and it's hard to be sure with our current buildsystem we're using the newer one from $PATH.

Hmm, we used to have grep calls which checked this. We must have dropped them along the way.

@jlebon
Copy link
Member

jlebon commented Feb 24, 2021

(And sorry for bringing this up now! I must have missed this PR.)

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.

5 participants