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

standardize path and behavior of ublue-os justfile #41

Closed
bsherman opened this issue May 13, 2023 · 20 comments · Fixed by #42
Closed

standardize path and behavior of ublue-os justfile #41

bsherman opened this issue May 13, 2023 · 20 comments · Fixed by #42
Assignees
Labels
enhancement New feature or request

Comments

@bsherman
Copy link
Contributor

bsherman commented May 13, 2023

@bsherman @EinoHR After some discussion on Discord, a good pattern emerged to avoid forks overwriting each other's justfiles randomly: Every image instead has its own separate justfile, which the user can include/merge a-la-carte.

Currently we have these:

touch ~/.justfile
cat /usr/share/ublue-os/just/main.just >> ~/.justfile
cat /usr/share/ublue-os/just/custom.just >> ~/.justfile

The main is meant to be the main config, which currently comes from the config repo. Its purpose is to contain the "core" commands.

The custom is from the startingpoint template repo, which is where the repo maker adds their own. It includes absolutely nothing from main and therefore never risks going out of sync with upstream ublue-os/config.

We'll need 4 minor changes in how the main justfile is built:

  • Target location: /usr/share/ublue-os/just/main.justfile
  • Script update: ublue-os-just.sh needs to use the new justfile path. The purpose of that script is currently to auto-install the justfile on login if the user (even root) doesn't have a ~/.justfile, and it does nothing if the user has the file. So its behavior is already fine. It only needs a tweak to the path. Well, it also uses illegal non-POSIX shell syntax (only works in bash and zsh), so I'll propose a fixed version below.
  • The script currently places itself in /etc/profile.d on the image which is incorrect. It needs to write itself to /usr/etc/profile.d, and from there "systemd" will take care of syncing it into the user's real /etc folder.
  • We may want to make it merge every OS-level justfile automatically, not just the main one. But that's tricky with a profile.d script since we can't know which shell we're running under and therefore can't use bashisms for example... so leaving that off for now.

Alright here's the fixed ublue-os-just.sh:

# Add uBlue's justfiles to user if they don't already have a justfile.
if [ ! -f "${HOME}/.justfile" ] && [ -f /usr/share/ublue-os/just/main.just ]; then
  cp /usr/share/ublue-os/just/main.justfile "${HOME}/.justfile"
fi

In time I will probably contribute to the other repos too, but at the moment I am still busy and uncomfortable with the structure of main, config and nvidia. I'm starting to study them though! :)

Originally posted by @Arcitec in ublue-os/base#149 (reply in thread)

@bsherman bsherman self-assigned this May 13, 2023
@bsherman bsherman added the enhancement New feature or request label May 13, 2023
@Arcitec
Copy link
Contributor

Arcitec commented May 13, 2023

I just noticed that Just officially uses the <something>.just suffix:

https://just.systems/man/en/chapter_52.html

Which is the suffix that everyone else will be using. So we should follow that, like this:

/usr/share/ublue-os/just/main.just
/usr/share/ublue-os/just/custom.just

The feature they have implemented in Just is awesome, by the way. It allows people to import as many modular just files as they want. The only issue is that it is still marked as "unstable" and only available if we run just via an extra flag. But that will change sometime soon. :)

I am in bed, but will take care of that rename in the startingpoint repo & the standardization discussion's summary post pretty soon, in 10 hours or so.

@xynydev
Copy link
Member

xynydev commented May 13, 2023

But that will change sometime soon.

The PR for passing unstable as an env var has been open for many many weeks now and I haven't seen any new commits in the just repo for the time I've been tracking it. I'm wondering if it would hurt to alias just='just --unstable', that was turned down earlier.

@Arcitec
Copy link
Contributor

Arcitec commented May 13, 2023

@EinoHR Yeah I heard about that feature being stalled. But I mean that even though they hide it behind a --unstable testing flag now, they're inevitably going to move it to "on by default" soon.

It's a really nice system btw, being able to include multiple cleanly separated "modules" of just commands.

As for aliasing... it's tricky, since people use many different shells which means many different locations to store such an alias. I think just recommending that people manually create that alias is enough. But the simplest for users atm is just merging things (cat >> ~/.justfile) as we currently recommend.

@Arcitec
Copy link
Contributor

Arcitec commented May 13, 2023

@bsherman For reference, I updated the original comment with the new, official upstream .just suffix:

https://github.com/orgs/ublue-os/discussions/149#discussioncomment-5889873

@bsherman
Copy link
Contributor Author

As for aliasing... it's tricky, since people use many different shells which means many different locations to store such an alias. I think just recommending that people manually create that alias is enough. But the simplest for users atm is just merging things (cat >> ~/.justfile) as we currently recommend.

I agree with merging until we get the official include feature from just.

@bsherman
Copy link
Contributor Author

bsherman commented May 13, 2023

@bsherman For reference, I updated the original comment with the new, official upstream .just suffix:

https://github.com/orgs/ublue-os/discussions/149#discussioncomment-5889873

Nice! since I used the "Reference in new issue" feature on the discussion comment, the text of this issue auto-updated!

Uh, scratch that... it did not LOL

I'm updating to match the reference. thank you for the alert.

@Arcitec @EinoHR does this mean I can implement this now?

@bsherman
Copy link
Contributor Author

I think i see an addition...

This is described as ublue-os-just.sh for main repo`:

# Add uBlue's justfiles to user if they don't already have a justfile.
if [ ! -f "${HOME}/.justfile" ] && [ -f /usr/share/ublue-os/just/main.just ]; then
  cp /usr/share/ublue-os/just/main.justfile "${HOME}/.justfile"
fi

I think we'd want to override this file in each downstream so they include their appropriate files

eg, in nvidia

if [ ! -f "${HOME}/.justfile" ] && [ -f /usr/share/ublue-os/just/main.just ]; then
  cp /usr/share/ublue-os/just/main.justfile "${HOME}/.justfile"
  cat /usr/share/ublue-os/just/cat.justfile >> "${HOME}/.justfile"
fi

and startingpoint would have something similar. this is the stop gap until just include is ready?

or... are we still doing an in Containfile appending of nvidia's extra justfile snippet to main.justfile until then?

@Arcitec
Copy link
Contributor

Arcitec commented May 13, 2023

does this mean I can implement this now?

@bsherman That would be awesome. :) Yes, startingpoint has these same justfile changes in https://github.com/ublue-os/startingpoint/pull/74/commits

So config/main is the only area behind.

I think i see an addition...

Yeah I mentioned that in the original comment:

We may want to make it merge every OS-level justfile automatically, not just the main one. But that's tricky with a profile.d script since we can't know which shell we're running under and therefore can't use bashisms for example... so leaving that off for now.

My thought is that we should probably just bother merging main, because in the end, users are gonna be using just's official include /full/path/main.just; include /full/path/nvidia.just etc modular inclusion syntax.

So anything we make now would be insanely short-lived and not worth the hassle?

@Arcitec
Copy link
Contributor

Arcitec commented May 13, 2023

or... are we still doing an in Containfile appending of nvidia's extra justfile snippet to main.justfile until then?

Hmm that's a rough question. Yeah let's make the nvidia image override that shell script for now, to do what you mentioned, where the nvidia image copies main and merges in nvidia. It's just a stopgap until the full just has include directive enabled by default so we can remove all these hacks.

@bsherman
Copy link
Contributor Author

or... are we still doing an in Containfile appending of nvidia's extra justfile snippet to main.justfile until then?

Hmm that's a rough question. Yeah let's make the nvidia image override that shell script for now, to do what you mentioned, where the nvidia image copies main and merges in nvidia. It's just a stopgap until the full just has include directive enabled by default so we can remove all these hacks.

Yeah, i'm good with this... it's either override the main.justfile or the profile script... and i think the latter is less ugly / clearer about intent for a user trying to read the scripts.

And agree, its just the stop gap.

@Arcitec
Copy link
Contributor

Arcitec commented May 13, 2023

@bsherman Exactly. Overriding the profile is clearer for intent and also future-proofed for when just has inclusion by default! :)

@bsherman
Copy link
Contributor Author

Engage hacker mode.

@Arcitec
Copy link
Contributor

Arcitec commented May 13, 2023

Good luck, friend! 🥂

@bsherman
Copy link
Contributor Author

@Arcitec one correction to your specification...

The script currently places itself in /etc/profile.d on the image which is incorrect. It needs to write itself to /usr/etc/profile.d, and from there "systemd" will take care of syncing it into the user's real /etc folder.

The rpm DOES use this path, but rpm-ostree puts that file in /usr/etc/ "magically" (i haven't read the code, but it does it somehow)...

If files in /usr/etc are not present in /etc they seem to get copied to /etc and are kept up to date as long as the /etc copy is not changed on the local system.

This is what we see with ostree admin config-diff.

Anyway, just pointing out I won't be implementing a change for this point and why.

@Arcitec
Copy link
Contributor

Arcitec commented May 13, 2023

@bsherman I was referring to this, as you can see it only puts it in /etc/:

bash-5.2# ls /usr/etc/profile.d/*ublue*
ls: cannot access '/usr/etc/profile.d/*ublue*': No such file or directory
bash-5.2# ls /etc/profile.d/*ublue*
/etc/profile.d/ublue-os-just.sh

But it seems like those files are being layered via rpm-ostree, which automatically tracks the /etc/ folder variant instead of putting a template in /usr/etc/, so yeah this seems fine. :)

@bsherman
Copy link
Contributor Author

bash-5.2# ls /usr/etc/profile.d/*ublue*
ls: cannot access '/usr/etc/profile.d/*ublue*': No such file or directory
bash-5.2# ls /etc/profile.d/*ublue*
/etc/profile.d/ublue-os-just.sh

But it seems like those files are being layered via rpm-ostree, which automatically tracks the /etc/ folder variant instead of putting a template in /usr/etc/, so yeah this seems fine. :)

Weird, because it seems different for me.

$ rpm -ql ublue-os-just
/etc/profile.d/ublue-os-just.sh
/usr/share/ublue-os/ublue-os-just
/usr/share/ublue-os/ublue-os-just/justfile
/usr/share/ublue-os/ublue-os-just/ublue-os-just.sh

$ ls -l /etc/profile.d/ublue-os-just.sh /usr/etc/profile.d/ublue-os-just.sh
-rwxr-xr-x. 1 root root 215 May 10 21:52 /etc/profile.d/ublue-os-just.sh
-rwxr-xr-x. 6 root root 215 Dec 31  1969 /usr/etc/profile.d/ublue-os-just.sh

$ diff /etc/profile.d/ublue-os-just.sh /usr/etc/profile.d/ublue-os-just.sh

@Arcitec
Copy link
Contributor

Arcitec commented May 13, 2023

@bsherman Yeah that's strange. The results I listed are what I am getting when I base a startingpoint image on ghcr.io/ublue-os/silverblue-main, and then inspecting the internal filesystem after building my derived image. Perhaps rpm-ostree (or ostree) moves the file when it derives images like that.

@bsherman
Copy link
Contributor Author

Is your output from running the image in podman or the rpm-ostree booted image? I think that would explain what you are seeing.

@Arcitec
Copy link
Contributor

Arcitec commented May 13, 2023

@bsherman From entering a bash shell in podman to look at the image contents. Yeah it could be something that gets fixed by later processes. :)

@Arcitec
Copy link
Contributor

Arcitec commented May 13, 2023

@bsherman Oh yeah you're right. On a Silverblue system that is rebased to silverblue-main, the file ends up in /usr/etc/profile.d too. Interesting. Learning new things every day about ostree. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants