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

Support defining external justfile locations through env var #1849

Closed
wants to merge 7 commits into from

Conversation

pprkut
Copy link

@pprkut pprkut commented Jan 15, 2024

This extends just so you can define multiple locations from which it tries to import other justfiles. This allows, for example, to put shared justfiles in a user defined global location once, rather than having to manage duplicates in every repo they are used in or deal with symlinks.

Example use: JUSTFILE_PATH="/path/to/external/justfiles/:./" just --list

This is my preferred solution for #1580 and while currently only implemented for imports, should work just as well for modules too, which might provide additional input or discussion points for #1799
I'm not set on the environment variable name, open for other suggestions.

Taking things slow as I'm fairly new to rust, but I think the idea should be clear 🙂

@casey
Copy link
Owner

casey commented Jan 17, 2024

Thanks for the PR! I actually think that expanding environment variables in paths, e.g., mod foo '$DIR/foo.just' might be a better approach here. A few thoughts:

  • If environment variables are in paths, then we can give a good error if the environment variable isn't set. Whereas if JUSTFILE_PATH isn't set, and that's where the module is, we can only give the generic module not found error.
  • There appears to be a very widely used crate for performing both ~ expansion and variable expansion called shellexpand. So adding this would just require adding that crate, and passing module paths into it.
  • shellexpand supports fallbacks, in the form of ${VAR:-/other/path}.
  • We should add this for mod statements first. My thinking here is that modules are still unstable, so breaking changes are acceptable, whereas imports are stable, so we can't add breaking changes. Technically, expanding variables in paths would be a breaking change, however, using $ or ~ in paths is so uncommon that I think it would be okay to do this, even for imports. However, doing mod first gives us a little more room to experiment.
  • Users can use different variables for different imports, instead of having a single variable for all imports.
  • Users can use variables in the middle of a path, i.e., /var/lib/${SERVICE}/mod.just.

What do you think?

@pprkut
Copy link
Author

pprkut commented Jan 29, 2024

Expanding the variables in the path was one of my suggestions in #1580, so that's fine with me too 🙂
The reason I went with an external path variable is that is was quicker for me to implement and get going, and provided the added benefit of being backwards compatible.

I get your argument for starting with mod first. I'm just not using modules so for my testing I'd have to do it for imports anyway. However, I don't mind if, once I have a good grip on the changes, the mod version of it gets merged first (maybe through a separate PR).

I'll have a look at shellexpand and see where I get. Thanks for the input! 🙂

Comment on lines +722 to +723
1 │ mod foo '$FOO/mod.just'
│ ^^^
Copy link
Author

Choose a reason for hiding this comment

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

The pointers here would be nicer on the path, but I couldn't figure out how to get to a token from relative

Copy link
Owner

Choose a reason for hiding this comment

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

Relative is an Option, but in the case of an error, you know there's a path, so you can unwrap it.

Copy link
Author

Choose a reason for hiding this comment

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

It's already unwrapped. The difficulty I face is that relative is a StringLiteral, and I need a Token in the error handling to position the pointers. I don't know how to get back from the literal string to the token.
I thought about perhaps using the module token and iterating to the next token after (something like module.token.next), but I don't see an easy way to do it this way either 😕

Copy link
Author

Choose a reason for hiding this comment

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

@casey Any remaining ideas on this? Otherwise I keep it as is 🙁

Copy link
Owner

Choose a reason for hiding this comment

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

The mod parser uses parse_string_literal, but there's also parse_string_literal token which returns both the string literal and the token, if you store the token in mod in addition to the string literal, you can use it in the error message.

src/compiler.rs Show resolved Hide resolved
Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

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

See review comments!

src/compiler.rs Outdated Show resolved Hide resolved
src/compiler.rs Outdated Show resolved Hide resolved
src/compiler.rs Show resolved Hide resolved
Comment on lines +722 to +723
1 │ mod foo '$FOO/mod.just'
│ ^^^
Copy link
Owner

Choose a reason for hiding this comment

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

Relative is an Option, but in the case of an error, you know there's a path, so you can unwrap it.

@casey
Copy link
Owner

casey commented May 19, 2024

I see some new commits. Is this ready for review?

@casey
Copy link
Owner

casey commented May 19, 2024

I just landed #2055, which adds a a new kind of string literal. Strings prefixed with x, i.e., x'$FOO', are now shell expanded, supporting $VAR, ${VAR}, ~, and ~USER . This shell expansion is performed at compile time, which means that it can be used anywhere a string literal is accepted, including import paths, module paths, and settings. I think this is a more general solution to the problem here, since it works everywhere, and since it requires using x, it's opt-in and thus backwards compatible. Sorry to waffle on this! If people find it un-ergonomic to put x before import paths, we can consider doing it by default in module and import paths, as explored in this PR.

@casey casey closed this May 19, 2024
@pprkut
Copy link
Author

pprkut commented May 19, 2024

Cool! No worries, I'm just happy the feature works 🙂

@fabiomoratti
Copy link

The shell expansion feature is very handy for managing different sets of variable for different environments, thanks for implementing it!

Il might be worth mentioning in the documentation that the expansion supports fallbacks: I was not familiar with the fallback feature and it took me a while (well, a lot of time, until I stumbled on this thread and the 17 Jan post about the merits of shellexpand) to figure out that it's possible to use the extension also if the env variable is not set.

Something like:

| Value | Replacement |
|------|-------------|
| $VAR | value of environment variable VAR |
| ${VAR} | value of environment variable VAR |
| ${VAR:-DEFAULT_VALUE} | value of environment variable VAR or DEFAULT_VALUE if the VAR variable is not set |
| Leading ~ | path to current user's home directory |
| Leading ~USER | path to USER's home directory |

In case someone is interested here is my strategy to manage multiple envs:

foo_default := "FOO_0"
bar_default := "BAR_0"

# The "envs" folder contains one <env-name>.env for each environment with the appropriate variables:
# env-1.env:
# ENV_FOO=FOO1
# ENV_BAR=BAR1
# 
# env-2.env:
# ENV_FOO=FOO2
# ENV_BAR=BAR2
# 
# If the JUST_ENV variable is not set load the default.env file, if it's missing use the hard coded values above
set dotenv-filename := x'envs/${JUST_ENV:-default}.env'

foo := env_var_or_default('ENV_FOO', foo_default)
bar := env_var_or_default('ENV_BAR', bar_default)

# Use foo and bar variables in the recipes

@casey
Copy link
Owner

casey commented Jun 13, 2024

@fabiomoratti Nice, I didn't know that ${VAR:-DEFAULT} was supported syntax. I opened #2153 to mention it in the readme.

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.

3 participants