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

Add task definitions for just as alternative command runner #127

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dalito
Copy link
Member

@dalito dalito commented Jan 7, 2025

Adds a justfile with tasks for the just command runner.

The added commands should match the current makefile. While working on this I slightly simplified the makefile/config, e.g. to use cookiecutter-parameters / env.vars more consistent.

Closes #120

@@ -22,6 +22,8 @@ generator_args:
mergeimports: true
jsonldcontext:
mergeimports: true
pydantic:
mergeimports: true
Copy link
Member

Choose a reason for hiding this comment

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

@dalito - can you confirm that this addition does not result in the pydantic version of the python serialization being generated automatically for new projects and overwriting the already generated dataclasses serialization? We definitely want to move to pydantic being the default python serialization eventually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it be OK to generate both standard python and pydantic? I have to try it out to be sure.

Copy link
Member

Choose a reason for hiding this comment

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

a couple of repos I'm aware of do that, yep (renaming the pydantic file to something different from the dataclasses version e.g.: https://github.com/biolink/biolink-model/blob/fe44b8d56b7a1e77fb1e94f78a143dddb8681565/Makefile#L127 -- the naming is kind of awkward but I bet we could come up with something more reasonable here).

but, it might be that we just want to split this change out from this PR? I will leave that to your judgment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this change.

Copy link
Member

@sierra-moxon sierra-moxon left a comment

Choose a reason for hiding this comment

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

I am not an expert at just - but I imagine the impetus for this PR is to help Windows users set up a functional project more easily?

I imagine for a new user it would be helpful to have a few more pointers on which to use: make or just (though I see good comments in the README about using just, in particular, for windows users).

Thank you for working all this out! its terrific :)

Copy link
Contributor

@amc-corey-cox amc-corey-cox left a comment

Choose a reason for hiding this comment

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

I'm generally in approval here for making things easier for people to develop on Windows. Unfortunately, I do have significant concerns about this opening new projects to having command runners out of sync. Can you discuss how we can mange this to avoid make and just acting differently over time in a new repo?


* **make or just as command runner**

The project contains a makefile but also a `justfile` with pre-defined complex commands. To execute these commands you either need `make` or [just](https://github.com/casey/just) as an alternative command runner. Especially for Windows users we suggest `just`. Install it by running:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned about having two command runners requiring changes in two places and the possibility of their getting out of sync. Do you have a strategy for managing both of these files to avoid that?

Copy link
Member Author

@dalito dalito Jan 8, 2025

Choose a reason for hiding this comment

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

Yes. I agree that having both is not ideal but may be worth an experiment. Some make users may also convert to just but it is difficult to predict.

# Load environment variables from config.public.mk or specified file
set dotenv-load := true
# set dotenv-filename := env_var_or_default("LINKML_ENVIRONMENT_FILENAME", "config.public.mk")
set dotenv-filename := x'${LINKML_ENVIRONMENT_FILENAME:-config.public.mk}'
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, you do import the make config here. Maybe my concern about two files is moot?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it reads from the same file as the make file. Only the tasks are defined again the just-way.

site: _gen-project _gendoc

# Deploy site
deploy: site
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, these do look like recreations of the same things in our Makefile. Or am I misunderstanding? Does just play nice with make or do we have two command runners with their own configs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Two runners, one config.

@dalito
Copy link
Member Author

dalito commented Jan 8, 2025

@sierra-moxon - I saw that you merged main into this (my) branch. Typically as an author that contributes from a fork, I would just rebase on current upstream main to get the latest changes and then force-push to update this PR. I think the reason why most projects prefer rebasing the PRs is the cleaner git history.

It would be great to merge #128 before this PR. Afterwards I would like to do a final check of all just commands in a Poetry 2.0 based project.

@sierra-moxon
Copy link
Member

sierra-moxon commented Jan 10, 2025

from dev call: consider pulling this functionality up one level, making code to create a justfile out of a makefile? or otherwise moving build system management back into python and away from make or just? (see ticket linked in this PR, probably hold off on merge until feedback from @dalito)

@dalito
Copy link
Member Author

dalito commented Jan 11, 2025

@sierra-moxon Thanks for the feedback from the dev call. Even if I created this PR I am also not convinced that another task runner is a good solution.

The main disadvantage of this PR is that it still requires access to bash (or sh) which comes with git-for-windows but needs to be put on path. Such small steps are often a hurdle for beginners. Something to note is that just can use other shells as well, e.g. powershell, but then the recipes would be platform-specific. It can also run commands in a Python shell which could solve the bash-shell-config issue. Here I tried to write the justfile makefile-alike for easy comparison. However, it would also be possible to write a justfile using python recipes.

To leave the code here in proper state, I did a fresh rebase and checked that the justfile commands actually work.

What I also recognized: The makefile expects a non-standard dotenv file (no quotes around strings) but just expects a standard-compliant dotenv file with quotes around strings with spaces. So just and make cannot use exactly the same dotenv config file. This is another argument against merging this PR (although the problem is make not just).

moving build system management back into python

Then I suggest to either
a) close this PR and work towards this "more in Python" goal or
b) merge it as "experimental" feature without long-term guarantees to gather feedback.

For b) the makefile should be made to work with standard dotenv files first to avoid incompatible configs. The incompatibility was introduced in #123 to work-around make problems.

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.

Alternative command runner to make (tasks/just/duty)
3 participants