Moved to makefile #2900

pksunkara · 2021-10-16T22:29:28Z

Moved to makefile to reduce duplication of feature list

Whenever we add an optional feature, we have to add it in several places. One of the root causes for that is the use of just which is not standard. Which means, we have to add instructions to CONTRIBUTING.md for normal usage without it. This in turn increases the mentions of optional features.

Actually lint with no features

Previously, the flag -p clap:3.0.0-beta.4 was removed from the command which is supposed to lint our repo when almost no features are enabled. This would not work at all because the other packages (which are now run) enable the default features for clap. So, I added it back. (Tested this thoroughly)

Makefile

epage · 2021-10-16T22:49:02Z

I was also playing in this area (just left it lying around as we focused on more other things).

In case this is of interest for ideas: #2901
(I've not resolved conflicts since last feature was added, didn't think it was worth it until we were focusing on this and now its not worth it until we decide on direction)

kbknapp · 2021-10-16T23:36:25Z

In general I'm not a huge fan of make because it lends itself to becoming inscrutable. I think that's part of the reason the rust repo moved from Make to Python (I'm not suggesting we use Python 😉). However if the goal is so that our CI is using this to iron out our feature complexity and such and this also makes it simple to replicate CI locally I think that's a good thing.

Unrelated, this makes me think that if Cargo started using claps env values more liberally we could also solve this with standard Cargo commands and a .env file.

pksunkara · 2021-10-16T23:38:25Z

Agree, I am not a fan either but this is the most common tool everyone has and we do need to replicate our feature complexity as you said

pksunkara · 2021-10-18T10:12:58Z

I am going to use this branch to do a bit of testing for github actions. So, don't mind all the pushes since they wouldn't be reviewed. I will add another comment when this is ready for review.

This was a kbknapp thing and he doesn't use it anymore. See clap-rs#2900.

epage · 2021-10-29T19:46:10Z

.github/workflows/ci-pr.yml

+      - name: Test (Minimal)
+        uses: actions-rs/cargo@v1
+        with:
+          command: test
+          args: ${{ needs.args.outputs.minimal }}
+      - name: Compile (Full)
+        uses: actions-rs/cargo@v1
+        with:
+          command: test
+          args: --no-run ${{ needs.args.outputs.full }}
+      - name: Test (Full)
+        uses: actions-rs/cargo@v1
+        with:
+          command: test
+          args: ${{ needs.args.outputs.full }}


Testing all of these in one job is the approach I originally took in #2802 but then backed away due to performance. How come you are moving back to that approach?

I saw that you moved them back in #2901 and thought maybe you wanted them back again. I will make them separate jobs.

I didn't move them back, I made them part of the matrix.

I stand corrected.

epage · 2021-10-29T19:47:11Z

.github/workflows/ci-pr.yml

    strategy:
      fail-fast: false
      matrix:
+        rust: [1.54.0, stable]


In #2802, we had agreed to only check the MSRV and not to do a full test run on it.

I wanted to check something but forgot to move it back.

epage · 2021-10-29T20:06:41Z

.github/workflows/ci.yml

+concurrency:
+    group: ci-${{ github.ref}}
+    cancel-in-progress: true
 jobs:


This isn't mentioned in a commit or the PR. Please elaborate on this is needed? I assume its unrelated to the rest of the changes and should be its own commit or even its own PR.

This is in there for the sake of cancelling CI runs when bors either crashes or we do bors r- on something that's running or on the general trying branch when we do bors try

Could you either

Split this out into its own PR

Split this out into its own commit

Call this out in the PR description

(ordered by preference, reverse order for amount of work :) )

epage · 2021-10-29T20:11:54Z

Makefile

@@ -0,0 +1,50 @@
+FEATURES_MINIMAL = --no-default-features --features 'std cargo' -p clap:3.0.0-beta.5


Previously, the flag -p clap:3.0.0-beta.4 was removed from the command which is supposed to lint our repo when almost no features are enabled. This would not work at all because the other packages (which are now run) enable the default features for clap. So, I added it back. (Tested this thoroughly)

Like with #2903, have we audited those dependencies to see if any should be removed?

clap_derive definitely needs derive dependency which we can't remove. So, we can't achieve minimal testing without providing -p clap:3.0.0-beta.5

Been giving this some thought over the weekend.

Especially as feature flags add/remove parts of the API (e.g. env), we need to test the whole monorepo against minimal and not just clap.

Some ideas

We leave off -p, assuming testing with derive is of a minimal risk from the loss of coverage of testing without it since it has little API or behavior impact (only if a plugin derived rather than hand-implemented a trait)

Move -p to the CI pipeline and have the job do a run without it and then a run with it (should have minimal overhead since it'll mostly be able to reuse build results)

Move the derive tests out of clap_derive and put a #![cfg(feature = "derive")] in then (since we treat clap_derive as more of an implementation detail of clap).

Also in thinking about this more, this is mostly independent of this PR (this PR just adds it back in one place and moves where the flag exists), so feel free to defer this if you want.

(Before I forget, I also suggest we switch to --manifest-path from -p so we don't to reduce number of places updated on version, bump, manual or automated, keeping the git history more focused on what is relevant for someone digging into it)

epage · 2021-10-29T20:12:57Z

Makefile

+	cargo test $(FEATURES_FULL)
+
+test:
+ifeq (3, $(words $(MAKECMDGOALS)))


Granted, my make experience was from an isolated set of developers but it seems like defining variables is more idiomatic than creating our own subcommand system

make test and make debug are about running a single test with the test file and the test name being arguments. Since the arguments can change any time, we can't put them in a variable.

People have cargo, I think its out of scope for us to be providing our own wrapper around it like this.

Using cargo means adding all the features we normally test and then making sure to specify the arguments to the test file correctly. I think the wrapper here helps.

How often does someone need to do that? We previously already talked about that we didn't need a just file, this is just a different flavor of the same thing.

As I said, make debug was pretty useful especially when testing the parsing logic bugs. I used it a lot during the default values bug fixing.

Thats fine if you find it useful, you always have the option of keeping your own collection of scripts that help you. They don't have to live in the repo.

I will delete it but I think others will find make debug just as useful.

I agree with @epage here. The common standard idiom here is to provide a variable in the makefile rather than programmatically checking how many arguments were provided... Best leave that to a shell script.

TEST_PKGS := clap_derive clap_up

Users can override those values with a .env file:

$ echo "TEST_PKGS=clap_derive" >> .env $ source .env $ make test

Then users who have differing opinions on what packages they want to test can maintain that .env file as part of their personal workflow.

The goal of a makefile is to provide a reproducible test environment. Personal scripts or makefiles should be held in a user's personal "helper utilities" repository, like a dotfiles repository. Alternatively if it's useful to a small subset of users, it can be placed in a shared contrib/ directory. But the goal of a "root" Makefile should be to provide a shared entrypoint to compile or test the project.

That being said... I'm curious why use a Makefile at all when cargo provides all of these features out of the box. You can tell what features cargo should test with cargo test --all-features, cargo test --no-default-features, and cargo test (with default features). Why define a FEATURES_FULL and FEATURES_MINIMAL when default features and --all-features handle these cases?

Makefile

epage · 2021-10-29T20:17:32Z

.github/workflows/ci-pr.yml

+      - id: minimal
+        name: Get args (Minimal)
+        run: echo "::set-output name=list::$(make features-minimal)"
+      - id: full
+        name: Get args (Full)
+        run: echo "::set-output name=list::$(make features-full)"


While I'm pleased with how past the job is (3s), I wish we had visualization to help determine the overall impact of these logistical jobs.

In weighing this proposed implementation complexity with #2901, I feel like the "warnings" isn't really buying us much, but is in fact is harmful (at best clogging up the screen with information not relevant to the current developer, at worst, the current developer things they are responsible and wastes time trying to figure out what to do).

There were a few extra PRs external contributors make after seeing these warnings on their screen in their PR. I would say these warnings have been net positive over the last year compared to them being shown at the end of the status check log. They are actually quite helpful sometimes when there's an error because the contributor doesn't have to open the log and scroll. The errors are visible at a glance.

Why are we allowing these warnings instead of turning them into errors, forcing them to be resolved in the relevant PR?

We do turn a few of the warnings into errors but there are others which are not really that impactful but the end users still see when using our library. We need to maintain a balance here.

Balance between what?

Balance between what warnings we uplevel as errors and what warnings to leave as such.

If its a warning clap_derive users will see by default, we should error on it a lot of people just turn warnings into errors in CI

Providing non-actionable feedback to a user is confusing and discouraging. Its helpful to display the warning to the person fixing introducing it but future contributors should not see it. I'd go as far as saying that if we don't value it enough to turn it into an error, I give more weight to the follow up contributor's experience.

Agreed. This is just kicking the can down the line. IMO a community contribution should be addressing linter errors in the first place.

Printing warnings in the console while compiling code locally during development makes sense because you shouldn't have to be expected to write perfect code while hacking away on something. But if you're asking someone to merge your PR into a project, it makes more sense to address and/or suppress those warnings.

As project maintainers you should expect a community member's contributions to meet a certain bar of code quality - marking PRs as failed on a code lint warning is one way to help ensure that. It also helps ensure the community has good quality code to reference as they learn from your project's own code, so in a sense it's another form of mentorship.

epage · 2021-11-01T16:20:54Z

Makefile

+clean:
+	cargo clean
+	find . -type f -name "*.orig" -exec rm {} \;
+	find . -type f -name "*.bk" -exec rm {} \;
+	find . -type f -name ".*~" -exec rm {} \;


Like with make debug and make test, this seems more user-specific than filling the role of use for CI or documenting how to contribute.

Agreed. This is a remnant of my single contributor days. As this project contributor based has grown, we should definitely consider only keeping the parts that make sense in order to contribute, not just a single users workflow (in this case mine 😜 )

epage · 2021-11-01T16:21:59Z

.github/workflows/ci.yml

        with:
          command: check
-          args: --target ${{ matrix.target }} --features "wrap_help yaml regex unstable-replace unstable-multicall unstable-grouped debug"
-      - name: Test release
+          args: --target ${{ matrix.target }} ${{ needs.args.outputs.full }} --features debug


If we are wanting to document for users, shouldn't we be getting the debug flag from the Makefile as well?

epage · 2021-11-01T16:22:30Z

.github/workflows/ci.yml

        uses: actions-rs/cargo@v1
        if: matrix.features == 'release'
        with:
          command: test
-          args: --target ${{ matrix.target }} --features "wrap_help yaml regex unstable-replace unstable-multicall unstable-grouped" --release
+          args: --target ${{ matrix.target }} ${{ needs.args.outputs.full }} --release


Similar, if we are documenting expectations, shouldn't we be providing this from the Makefile

epage · 2021-11-01T16:26:36Z

.github/workflows/ci.yml

+      - name: Test (Full)
        uses: actions-rs/cargo@v1
-        if: matrix.features == 'all'
+        if: matrix.features == 'full'
        with:
          command: test
-          args: --target ${{ matrix.target }} --features "wrap_help yaml regex unstable-replace unstable-multicall unstable-grouped"
+          args: --target ${{ matrix.target }} ${{ needs.args.outputs.full }}


In #2901, you mentioned cherry-picking changes over. With the simplified pipeline that offers, I had switched us over to separating compilation from testing, like ci-pr.yml (which I originally didn't do to ci.yml because the logic was complicated as-is).

Where do you see the trade off of that now since this doesn't include cherry-picking that change and doesn't comment on it?

epage · 2021-11-01T16:29:17Z

Makefile

@@ -0,0 +1,50 @@
+FEATURES_MINIMAL = --no-default-features --features 'std cargo' -p clap:3.0.0-beta.5
+FEATURES_FULL = --features 'wrap_help yaml regex unstable-replace unstable-multicall unstable-grouped'
+


One of the things I did in #2901 was document design guidelines for people to keep in mind. Should we bring those over and add any for the documentation role this file fills (like "only include non-CI features that are intended to document the CI for contributors")?

I know the original comments are of a little lower value since we aren't calling into this from CI as much as we are in this PR.

epage · 2021-11-01T16:29:57Z

Makefile

+features-minimal:
+	@echo "$(FEATURES_MINIMAL)"
+
+features-full:
+	@echo "$(FEATURES_FULL)"


Should we prefix these feature names with list-? A couple of times, I was thrown off by the names when looking around this PR.

kbknapp · 2021-11-03T01:01:47Z

It's too much for this PR, but I feel we're getting to the level that we may need to introduce some additional tooling or best practices to deal with the two things we have going on in this monorepo:

A large number of cargo features
A growing number of sub-crates

It'd be worth spending some time looking at other large multi-crate repositories to see how they're structuring things. One that comes to mind is Tokio, also having a large number of features and sub-crates.

Additionally, looking at tooling like cargo-hakari to see if they can be of any use. I'm not suggesting we adopt them, but just to look at them.

I understand make is ubiquitous enough that we can assume people already have it installed, however we can also assume people have cargo and thus I'm 100% OK with our contribution documentation saying, cargo install XYZ, whether that's just, cargo-make, cargo-hakari, or any other number of tools we could utilize to make this repository better. I would draw the line at having to install tools outside of cargo install or a distributions build-essentials (or equivalent meta package).

Essentially, I'd like to pick the best tool for the job and what makes our lives, our contributors lives, and CI easier to run and maintain.

epage · 2021-11-03T01:32:02Z

Again, too much for this PR but another tool I've seen in some cases is cargo-hack which allows running limited commands across all feature combinations. Doing this for tests might be a bit much due to link and test times, but I don't expect many tests to be impacted by the combination of features. Running it for check might be interesting to make sure they at can compile in combination with each other.

kbknapp · 2021-11-03T15:48:24Z

@pksunkara I'm going to propose we tag this PR with a "WIP" or "Draft" tag so that it's not confused with v3 blocking PRs. You're more than welcome to continue to hack away at this PR along or experiment with the various suggestions but there is nothing inherently tied to v3 in this PR so I don't see any harm if you need to take time to try things out and it were to merge post release.

epage · 2021-12-07T22:32:58Z

A version of a makefile has been added, assuming it supersedes this work. Feel free to re-open and rebase if you disagree.

ofek · 2022-10-28T15:51:33Z

fyi make doesn't work on Windows without a lot of extra effort

epage · 2022-10-29T12:52:11Z

The priority for the Makefile is CI, contributors come second. Contributors are not required to use Makefiles but they are available as a convenience for reproducing CI. You can look in the Makefile and reproduce what it does.

epage reviewed Oct 16, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

epage reviewed Oct 16, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

pksunkara mentioned this pull request Oct 17, 2021

chore(ci): Re-use CI logic through make #2901

Closed

pksunkara marked this pull request as draft October 18, 2021 10:13

epage added a commit to epage/clap that referenced this pull request Oct 23, 2021

chore: Remove unused justfile

5256071

This was a kbknapp thing and he doesn't use it anymore. See clap-rs#2900.

epage mentioned this pull request Oct 23, 2021

chore: Remove unused justfile #2923

Merged

epage added a commit to epage/clap that referenced this pull request Oct 23, 2021

chore: Remove unused justfile

3ceb3c2

This was a kbknapp thing and he doesn't use it anymore. See clap-rs#2900.

pksunkara force-pushed the makefile branch 5 times, most recently from 90f7c60 to d2e047e Compare October 28, 2021 17:15

pksunkara added 2 commits October 28, 2021 20:41

Moved to makefile to reduce duplication of feature list

a6f2161

Reduce duplication of features arg for CI

c65ae46

pksunkara force-pushed the makefile branch from d2e047e to c65ae46 Compare October 28, 2021 19:43

pksunkara marked this pull request as ready for review October 28, 2021 19:47

pksunkara requested a review from epage October 28, 2021 19:47

epage reviewed Oct 29, 2021

View reviewed changes

Makefile Show resolved Hide resolved

epage reviewed Oct 29, 2021

View reviewed changes

epage reviewed Nov 1, 2021

View reviewed changes

kbknapp marked this pull request as draft November 16, 2021 15:34

epage closed this Dec 7, 2021

pksunkara deleted the makefile branch February 10, 2022 23:35

		@@ -0,0 +1,50 @@
		FEATURES_MINIMAL = --no-default-features --features 'std cargo' -p clap:3.0.0-beta.5

		@@ -0,0 +1,50 @@
		FEATURES_MINIMAL = --no-default-features --features 'std cargo' -p clap:3.0.0-beta.5
		FEATURES_FULL = --features 'wrap_help yaml regex unstable-replace unstable-multicall unstable-grouped'

Moved to makefile #2900

Moved to makefile #2900

Conversation

pksunkara commented Oct 16, 2021 • edited by epage Loading

epage commented Oct 16, 2021 • edited Loading

kbknapp commented Oct 16, 2021

pksunkara commented Oct 16, 2021

pksunkara commented Oct 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pksunkara Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pksunkara Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bacongobbler Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bacongobbler Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbknapp commented Nov 3, 2021

epage commented Nov 3, 2021

kbknapp commented Nov 3, 2021 • edited Loading

epage commented Dec 7, 2021

ofek commented Oct 28, 2022

epage commented Oct 29, 2022

pksunkara commented Oct 16, 2021 •

edited by epage

Loading

epage commented Oct 16, 2021 •

edited

Loading

pksunkara Oct 29, 2021 •

edited

Loading

pksunkara Oct 29, 2021 •

edited

Loading

bacongobbler Nov 5, 2021 •

edited

Loading

bacongobbler Nov 5, 2021 •

edited

Loading

kbknapp commented Nov 3, 2021 •

edited

Loading