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

Fix/enhance CICD #2750

Merged
merged 19 commits into from Nov 20, 2021
Merged

Fix/enhance CICD #2750

merged 19 commits into from Nov 20, 2021

Conversation

rivy
Copy link
Member

@rivy rivy commented Nov 17, 2021

This PR contains several fixes/enhancements for CI...

  • style updates

    • style steps can now be globally and individually configured to fail the build on error or report-only
    • convert the cargo udep ... check into a style
  • fix clippy and spelling style lints

    • fix dashboard display of cargo clippy lint complaints
    • fix broken cspell
  • fix FreeBSD test

    FreeBSD testing only functioned if the repository name was 'coreutils'. If the repository had another name, the step broke. It's now been revised into a more robust form which will function within any repository.

  • fix MacOS code coverage

    Recent commits to 'coreutils' use the 'selinux' crate which forces the use of the 'nix' v0.23+ crate which, unfortunately, has a number of recent changes which break the MacOS coverage build and the 'uu_yes' sub-crate.

The last commit is an exploration of adding dependencies between CI steps; eg, the build/test steps won't run until dependency and MSRV checks pass, and code coverage won't run until/unless the build steps all pass.

  • More efficient in not burning CPU cycles when the build is failing.
  • Slightly better look on the dashboard?
  • Hate it? Like it?
  • Easy to drop if everyone hates it.

I'll convert from draft to final PR when the yes change is ok'd and we make a decision about whether to keep the new step dependencies.

@rivy rivy marked this pull request as draft November 17, 2021 12:15
@blyxxyz
Copy link
Contributor

blyxxyz commented Nov 17, 2021

I believe I captured the correct gist of the code and all tests still pass, but @blyxxyz , would you mind looking at commit (4ef2faf) and let me know if it needs a change?

That change looks good. My old code was questionable, the new Nix errors make it saner.

@sylvestre
Copy link
Sponsor Contributor

It would be easier to review if you could split PR into smaller PR. Don't bother for this one but for the future, please consider it!

The last commit is an exploration of adding dependencies between CI steps; eg, the build/test steps won't run until dependency and MSRV checks pass, and code coverage won't run until/unless the build steps all pass.

I love this idea

@sylvestre
Copy link
Sponsor Contributor

And sad about the vendoring :(

@rivy
Copy link
Member Author

rivy commented Nov 19, 2021

It would be easier to review if you could split PR into smaller PR. Don't bother for this one but for the future, please consider it!

I agree that it's too big, but it's really the vendoring of 'nix' that makes this huge... the primary core of the change is much more reasonable. Look at master...39a6e6c for a better overview not cluttered by the vendoring. Not small, but I hope/think reasonable, and all logically connected.

The last commit is an exploration of adding dependencies between CI steps; eg, the build/test steps won't run until dependency and MSRV checks pass, and code coverage won't run until/unless the build steps all pass.

I love this idea

Then I'll make it a change instead of an experiment.

And sad about the vendoring :(

I should be able to remove the vendored code upon the release of nix-v0.24.0 (which should have feature-gated functionality; see nix-rust/nix#1588 (comment) and nix-rust/nix#1498). We'll just need to evaluate the used functionality and enable that subset of features within the Cargo.toml of the crates using 'nix'; that should avoid the problem.

- fixes conversion of new `cargo clippy` output style to GHA annotations

## [why]

`cargo clippy` output formatting changed, using relative instead of absolute paths.
- add individual job-step control for 'style' step faults (build failure vs only a warning)
# [why]

The tool cache is currently failing and seems to be getting further behind current
versions. The [actions-rs/install#12] issue addresses this but seems to be
languishing without any proposed solution.

[ref]: <actions-rs/install#12>
….2.8

## [why]

`cspell` in CI started mysteriously failing mid-2021. Tracking down the
error took some time as it was not obvious from `cspell` feedback where
the issue lay. Ultimately, it was discovered that `cspell` had deprecated
use on NodeJS versions < v12 for `cspell` v5+.

`cspell` is now pinned to v4.2.8, with a maintenance note to allow an
upgrade to the `cspell` version when a version of NodeJS >= v12 is being
used in the CI.

An issue requesting better tool feedback for similar situations was also
opened on the `cspell` repo.[*]

[*]: [🙏🏻 Add warning (or error) when used on deprecated/outdated JS platform versions](streetsidesoftware/cspell#1984)
- the build and test steps won't run until/unless Dependency and MSRV checks pass
- code coverage won't run until/unless the build steps all pass

## [why]

This helps make more efficient use of CI resources and can help more easily visualize
build issues from the resultant GHA dashboard flow diagram.
@rivy
Copy link
Member Author

rivy commented Nov 20, 2021

That change looks good. My old code was questionable, the new Nix errors make it saner.

@blyxxyz , thanks for the look.

@rivy
Copy link
Member Author

rivy commented Nov 20, 2021

Once tests on the rebase pass, I'll change this from draft to ready.

@rivy rivy marked this pull request as ready for review November 20, 2021 06:17
@sylvestre sylvestre merged commit 4d33a3a into uutils:master Nov 20, 2021
@rivy rivy deleted the fix.cicd branch January 7, 2022 23:22
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