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

Generate completions at compile-time #118

Merged
merged 9 commits into from
Apr 2, 2023

Conversation

TD-Sky
Copy link
Contributor

@TD-Sky TD-Sky commented Mar 25, 2023

There are two ways to generate completions in clap:

  • Provide a subcommand or flag to generate completion script of specified shell in runtime;
  • Generate automatically by build.rs at compile-time.

I observe that some projects didn't add completion scripts generation at the beginning. So when they wanted to generate completion scripts at compile-time, they would face a tough work of bringing types related to the command parser all together. At this point, they tend to choose the runtime generation.

Therefore, I rewrite the generation to run in compile-time as dotter doesn't have the difficulties above. At the same time I refactor the workflows to pack completion scripts and binary together. If this PR passes, I will make new PKGBUILD and pull request to orhun.


When I was debugging workflows, I found that I had to turn on Settings > Code and automation > Actions > General > Workflow permissions > Read and write permission. Did you turn off it? Or is this the default setting for every new fork repository?

Copy link
Owner

@SuperCuber SuperCuber left a comment

Choose a reason for hiding this comment

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

Hey, sounds like a good idea :) Couple points added in comments.

  • Can you make a release on your repo to see the result of the run?

When I was debugging workflows, I found that I had to turn on Settings > Code and automation > Actions > General > Workflow permissions > Read and write permission. Did you turn off it? Or is this the default setting for every new fork repository?

It's on for this repo and I don't think I messed with it, maybe they changed the default 🤷

@TD-Sky
Copy link
Contributor Author

TD-Sky commented Mar 30, 2023

Hey, sounds like a good idea :) Couple points added in comments.

* Can you make a release on your repo to see the result of the run?

When I was debugging workflows, I found that I had to turn on Settings > Code and automation > Actions > General > Workflow permissions > Read and write permission. Did you turn off it? Or is this the default setting for every new fork repository?

It's on for this repo and I don't think I messed with it, maybe they changed the default shrug

Please refer to https://github.com/TD-Sky/dotter/actions/runs/4564049786 .

I set the workflow running event to push tags. Now we can use git push --tags in command line to start the workflow.

@SuperCuber
Copy link
Owner

I set the workflow running event to push tags. Now we can use git push --tags in command line to start the workflow.

What happens to the uploaded assets if a tag is created without a release?

@TD-Sky
Copy link
Contributor Author

TD-Sky commented Mar 31, 2023

I set the workflow running event to push tags. Now we can use git push --tags in command line to start the workflow.

What happens to the uploaded assets if a tag is created without a release?

Did you mean you dislike releasing on tag created?

Once the tag is created, the action will definitely start and create new assets.

@SuperCuber
Copy link
Owner

the action will start and create new assets

but will they be uploaded and attached to the Release? I assume that if I create a release+tag at the same time then it will work, but I'm wondering about a tag only since you're suggesting it

@TD-Sky
Copy link
Contributor Author

TD-Sky commented Mar 31, 2023

the action will start and create new assets

but will they be uploaded and attached to the Release? I assume that if I create a release+tag at the same time then it will work, but I'm wondering about a tag only since you're suggesting it

Certainly yes, that is what I've always done in my project.

@TD-Sky TD-Sky force-pushed the build-completions branch from 46126bc to e5cfb14 Compare April 2, 2023 03:49
@TD-Sky
Copy link
Contributor Author

TD-Sky commented Apr 2, 2023

Copy link
Owner

@SuperCuber SuperCuber left a comment

Choose a reason for hiding this comment

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

Looks good! Are the completions supposed to be checked into the repo? If they are generated I feel like it should be gitignored.

Also please fix CI fails :)

@TD-Sky
Copy link
Contributor Author

TD-Sky commented Apr 2, 2023

Looks good! Are the completions supposed to be checked into the repo? If they are generated I feel like it should be gitignored.

Also please fix CI fails :)

I think there are no any strict standards about whether to track completion files. So ignoring them is OK.

The clippy failure came from the old codes. Are you sure letting me fix it?

@SuperCuber
Copy link
Owner

The warning comes from the build.rs using the args file but not all the functions.

Make sure to first remove the files then gitignore, git makes this annoying

@TD-Sky
Copy link
Contributor Author

TD-Sky commented Apr 2, 2023

Done.

README.md Outdated
@@ -48,7 +48,6 @@ Commands:
undeploy Delete all deployed files from their target locations. Note that this operates on all files that are currently in cache
init Initialize global.toml with a single package containing all the files in the current directory pointing to a dummy value and a local.toml that selects that package
watch Run continuously, watching the repository for changes and deploying as soon as they happen. Can be ran with `--dry-run`
gen-completions Generate shell completions
Copy link
Owner

Choose a reason for hiding this comment

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

needs revert

@TD-Sky
Copy link
Contributor Author

TD-Sky commented Apr 2, 2023

dotter is using packages that are going to be out of date warning: the following packages contain code that will be rejected by a future version of Rust: nom v1.2.4, nom v5.1.2, terminfo v0.7.3, which caused CI failed.

@SuperCuber
Copy link
Owner

yeah that's because of watchexec and upgrading that is a pain cuz breaking changes 😠
it doesn't fail CI though, only warning for now

@TD-Sky
Copy link
Contributor Author

TD-Sky commented Apr 2, 2023

yeah that's because of watchexec and upgrading that is a pain cuz breaking changes angry it doesn't fail CI though, only warning for now

Sorry, I didn't see the Error below.

The crate meval has been given up for a long time. Do you have other alternatives?

@TD-Sky
Copy link
Contributor Author

TD-Sky commented Apr 2, 2023

I track the crate in the project with cargo tree -i <package>. For example, cargo tree -i [email protected] lets me know that meval is using it.

@SuperCuber
Copy link
Owner

https://lib.rs/crates/exmex
https://lib.rs/crates/evalexpr
https://lib.rs/crates/kalk

found those with a quick search.

I appreciate the help with the warning but let's split that into a separate PR (it's already mentioned in #110 too)

@TD-Sky
Copy link
Contributor Author

TD-Sky commented Apr 2, 2023

OK, I got it.

@SuperCuber SuperCuber merged commit fd4ef23 into SuperCuber:master Apr 2, 2023
@SuperCuber
Copy link
Owner

Thanks for the contribution <3 I'll make a release later today

SuperCuber added a commit that referenced this pull request Apr 2, 2023
@SuperCuber
Copy link
Owner

SuperCuber commented Apr 2, 2023

(ignore revert, misclick)

Seems like cargo publish will not allow a crate that modifies files outside of the value of OUT_DIR environment variable, see
rust-lang/cargo#5073
https://doc.rust-lang.org/cargo/reference/build-scripts.html#outputs-of-the-build-script

Found this out using cargo publish --dry-run, will add it to the CI I think.

@SuperCuber
Copy link
Owner

@TD-Sky not sure if you're getting pinged since the PR is closed, I tried searching for a solution but I'm not sure what to do about this... Maybe we can ditch the build.rs and instead run dotter gen-completions in CI for each shell?

@TD-Sky
Copy link
Contributor Author

TD-Sky commented Apr 4, 2023

@SuperCuber I remember I had this problem before. I had to track completions to solve it. Sorry, it has been too long since I had published crate so that I forgot about it.

Calling gen-completion at the packing stage is OK. Actually yesterday I found that dotter-rs in AUR would generatescompletion files in this way.

Choosing which scheme depends on you.


I found didyoumean also ignores the compile-time generated files. But they publish to crates.io successfully! Let me ask how they do that.

@TD-Sky
Copy link
Contributor Author

TD-Sky commented Apr 4, 2023

If one PR is closed, the participants would not receive the new comments unless they are @.

@SuperCuber
Copy link
Owner

@TD-Sky Yeah, strange how it works for them. Maybe they use --no-verify when publishing, which sounds like it is against best practices so I wouldn't do that.

Calling gen-completion at the packing stage is OK

I meant calling it in publish CI job and then uploading it to the release, but at packing stage is also an option.

@TD-Sky
Copy link
Contributor Author

TD-Sky commented Apr 9, 2023

@SuperCuber The author of didyoumean has been inactive for a long time (the last activity is at the end of last year).

Maybe they use --no-verify when publishing, which sounds like it is against best practices so I wouldn't do that.

I guess you are right. He didn't make a CI for cargo publish so he maybe check and publish manually.

Now there are two solutions:

  • Track completions;
  • Generate at runtime in CI, but this PR should be drop.

However, I don't understand why orhun only generates completions for dotter-rs and not for dotter-rs-bin.

@SuperCuber
Copy link
Owner

@TD-Sky I just noticed that I did not get a notification for your message 🤦 i think generating in CI is the best approach. Let's see if the Revert button on the PR works :)

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.

2 participants