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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move from doit to just #160

Merged
merged 9 commits into from Nov 4, 2021
Merged

Move from doit to just #160

merged 9 commits into from Nov 4, 2021

Conversation

FollowTheProcess
Copy link
Contributor

Related to discussion item: #128

This PR adds a justfile to the root of the project with targets taken from the dodo.py configuration file.

I've run all the recipes locally and they seem to be working fine 馃檪

There's a few differences between this and the dodo.py implementation:

  • Because join currently only accepts 2 arguments (pending next release) I've had to declare a few more variables to get proper file paths
  • I've changed the default task to just print the task list while just is still new to the project (happy to change to reflect default tasks in dodo.py, thought I'd keep it safe for now!)
  • I tried to get the Graphviz target running in a for loop similar to dodo.py so you could just pass an array of formats but couldn't get just's variable substitution and shell variable expansion playing together nicely so I've just got it as two lines in the task for now

I appreciate I didn't give you chance to respond in the discussion before throwing this PR at you but I was bored and this seemed like fun!

Please feel free to reject if this is no longer wanted 馃檪

Thanks!

@brettcannon brettcannon self-requested a review October 25, 2021 22:29
@brettcannon
Copy link
Owner

Thanks for the PR! I will have a look when I get a chance.

@brettcannon
Copy link
Owner

brettcannon commented Oct 25, 2021

@FollowTheProcess Could you also update the GitHub Actions workflows so I can see what it will look like to use in real-world scenarios?

@FollowTheProcess
Copy link
Contributor Author

FollowTheProcess commented Oct 26, 2021

@FollowTheProcess Could you also update the GitHub Actions workflows so I can see what it will look like to use in real-world scenarios?

Sure!,I'll take a look at that!

I notice I failed the changelog workflow too so I'll sort that out 馃憤馃徎

Copy link
Owner

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Initial CI run failed due to case-sensitivity issues.

justfile Outdated Show resolved Hide resolved
@brettcannon
Copy link
Owner

You don't have to worry about this until I have had time to look at the justfile and make a call as whether to take this overall change, but CONTRIBUTING.md will also need an update.

Co-authored-by: Brett Cannon <brett@python.org>
@FollowTheProcess
Copy link
Contributor Author

FollowTheProcess commented Oct 26, 2021

You don't have to worry about this until I have had time to look at the justfile and make a call as whether to take this overall change, but CONTRIBUTING.md will also need an update.

Cool, no worries! I could take the changelog fragment out then if you like?

Was a nice little intro to scriv anyway, I've not used it before!

Edit: Just thinking how I managed to miss the case issue in Cargo.toml as running just worked on my machine. I think my shell (fish) is case insensitive with regard to file paths where as bash (as in the GHA runner) is not? Potentially something to watch out for using something like just over doit who's paths are all taken care of by python and pathlib 馃憤馃徎

@brettcannon
Copy link
Owner

brettcannon commented Oct 26, 2021

I think my shell (fish) is case insensitive with regard to file paths where as bash (as in the GHA runner) is not?

What OS are you running under? I'm a fish user myself but I have never run into any case-insensitive aspects from the shell. Typically case-sensitivity is a file system thing and both Windows and macOS are case-insensitive by default (hence why the GHA runners are case-sensitive since they are run under Linux). But I'm not too concerned about having to watch out for anything since that's why there's CI. 馃槈

@brettcannon
Copy link
Owner

Cool, no worries! I could take the changelog fragment out then if you like?

Nope, fragment is good! The contributing documentation will just also need an update since it mentions doit.

@brettcannon brettcannon self-requested a review October 26, 2021 19:48
@FollowTheProcess
Copy link
Contributor Author

Typically case-sensitivity is a file system thing and both Windows and macOS are case-insensitive by default (hence why the GHA runners are case-sensitive since they are run under Linux).

TIL! I'm on macOS, I always thought it was a shell thing.

Nope, fragment is good! The contributing documentation will just also need an update since it mentions doit.

Cool, this is my next job then 馃檪

Copy link
Owner

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Looks great! I have a couple of suggestions, but I'm not sure if they make sense since this is my first justfile. 馃槄

.github/workflows/main.yml Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
changelog.d/20211026_081619_tomfleet2018_use_just.md Outdated Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
justfile Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
@FollowTheProcess
Copy link
Contributor Author

FollowTheProcess commented Oct 30, 2021

@brettcannon Think I got all your suggestions in, let me know if I missed any. My only question was re the use of the walrus in the python script around maybe explaining somewhere that a contributor would need python >=3.8 to run this recipe?

@brettcannon
Copy link
Owner

Just realized that .gitignore also now has a dead entry (found via https://github.com/brettcannon/python-launcher/search?q=doit).

justfile Outdated Show resolved Hide resolved
justfile Outdated Show resolved Hide resolved
FollowTheProcess and others added 2 commits November 2, 2021 09:10
Co-authored-by: Brett Cannon <brett@python.org>
Co-authored-by: Brett Cannon <brett@python.org>
@FollowTheProcess
Copy link
Contributor Author

FollowTheProcess commented Nov 2, 2021

Just realized that .gitignore also now has a dead entry (found via https://github.com/brettcannon/python-launcher/search?q=doit).

Yeah I left that in because I didn't know if you wanted to completely remove doit? Happy to take it out? Would you want dodo.py removing as part of this too?

I'll update the Dockerfile too 馃憤馃徎

Edit: On the Dockerfile, I can't see an entry in just's install docs referring to Debian although my Linux knowledge isn't great!

As far as I can see the best option might be to cargo install it?

@brettcannon
Copy link
Owner

Yeah I left that in because I didn't know if you wanted to completely remove doit? Happy to take it out? Would you want dodo.py removing as part of this too?

Yep, this PR is removing all use of doit from the project.

I'll update the Dockerfile too 馃憤馃徎

Thanks! And I'm not Docker expert, so if there's anything missing please let me know.

As far as I can see the best option might be to cargo install it?

Yep, cargo install just is good enough.

@FollowTheProcess
Copy link
Contributor Author

So this is all done with a few tweaks:

I've had to cargo install just as the vscode user as doing so under root causes a permission error when running any just recipe that touches cargo (presumably because the user is trying to modify/execute stuff that was created under root?):

image

Second, it seems that the default python in the devcontainer is 3.7.3 so the walrus does indeed fail!

image

image

I'm not totally sure how to introduce another version of python, I don't think it's possible to just include something like FROM python3:slim-buster?

The only other way I could think is building from source but this seems complex and lengthy for a devcontainer? Are you aware of another way?

For now I've just used the 3.7 style syntax to get it working and pushed up, I'll have a look into it in the meantime 馃憤馃徎

@brettcannon
Copy link
Owner

It's fine to leave out the walrus operator; I'm too lazy to update the dev container just to support it right now. 馃槃

When you're ready for another review, feel free to refresh my review so it ends back up into my GitHub review queue.

@FollowTheProcess
Copy link
Contributor Author

It's fine to leave out the walrus operator; I'm too lazy to update the dev container just to support it right now. 馃槃

In that case I think it should be good to go!

@brettcannon brettcannon merged commit f8266ef into brettcannon:main Nov 4, 2021
@brettcannon
Copy link
Owner

Thanks for all your work on this, @FollowTheProcess !

@FollowTheProcess
Copy link
Contributor Author

Thanks for all your work on this, @FollowTheProcess !

Happy to help! py is my go to these days, thanks for your time helping and reviewing 馃帀

@FollowTheProcess FollowTheProcess deleted the use-just branch November 4, 2021 08:10
@brettcannon brettcannon added the impact-maintenance Maintenance of the code/project label Mar 4, 2022
@brettcannon brettcannon changed the title Add a justfile with targets from doit Move from doit to just Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact-maintenance Maintenance of the code/project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants