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

refactor: use standard path for just plus... #42

Merged
merged 3 commits into from
May 14, 2023

Conversation

bsherman
Copy link
Contributor

@bsherman bsherman commented May 13, 2023

This changes the ublue-os-just RPM by:

  • using /usr/share/ublue-os/just path (no double ublue-os)
  • renames "justfile" to "main.just"
  • adds "nvidia.just" and "custom.just"
  • improves profile script to smartly add appropriate just files

Closes #41

This changes the ublue-os-just RPM by:
- using /usr/share/ublue-os/just path (no double ublue-os)
- renames "justfile" to "main.just"
- adds "nvidial.just" and "custom.just"
- improves profile script to smartly add appropriate just files

Closes #41
@bsherman bsherman linked an issue May 13, 2023 that may be closed by this pull request
@Arcitec
Copy link
Contributor

Arcitec commented May 13, 2023

Woo! This is looking really awesome! :)

  1. Should we include nvidia.just here in the package? Doesn't the addition of that file belong to our NVIDIA repo? Although I suppose it's easier to manage it here! And you did a great job with the nvidia rpm check to make sure only NVIDIA users get it! :)
  2. The custom.just is added by startingpoint during layering, so I was surprised to see a blank one being included in the RPM here. But it actually seems kind of nice to put that blank dummy file here, to "enforce that rule". It's a harmless empty file if people don't use startingpoint and it can give people ideas if they build their own containers manually... so I like it. :)
  3. The patching method of the profile.d script needs some fixes for non-bash shells (for example zsh cannot append to non-existent files which is why we must touch first, and we can't trust that $HOME has a value which would lead the current script to attempt a write to /.justfile), and we could also avoid creating those permanent global variables when they aren't needed. Posting below. Feel free to change anything below! :)
# Only process users with home directories, who are lacking a justfile.
if [ ! -z "$HOME" ] && [ -d "$HOME" ] && [ ! -f "${HOME}/.justfile" ]; then
  UBLUE_JUST=/usr/share/ublue-os/just
  USER_JUSTFILE="${HOME}/.justfile"
  touch "${USER_JUSTFILE}"
  if [ -f ${UBLUE_JUST}/main.just ]; then
    cat ${UBLUE_JUST}/main.just >> "${USER_JUSTFILE}"
  fi
  if [ -f ${UBLUE_JUST}/nvidia.just ] && [ rpm -q xorg-x11-drv-nvidia ]; then
    cat ${UBLUE_JUST}/nvidia.just >> "${USER_JUSTFILE}"
  fi
  if [ -f ${UBLUE_JUST}/custom.just ]; then
    cat ${UBLUE_JUST}/custom.just >> "${USER_JUSTFILE}"
  fi
fi

This would work on every shell. :)

@bsherman
Copy link
Contributor Author

this makes sense except for this line:

cat ${UBLUE_JUST}/main.justfile >> "${USER_JUSTFILE}"

I was originally using cp which should work in bash or zsh and should eliminate need for touch, unless you are trying to cover the edge case where main.just doesn't exist, which really can't happen unless users remove the main.just file but keep using the profile script.

@Arcitec
Copy link
Contributor

Arcitec commented May 13, 2023

unless you are trying to cover the edge case where main.just doesn't exist

Yep, it was just for the paranoid scenario where someone makes their own image without main.just existing anymore, maybe they add a rm command in their own scripts to delete it for example.

@bsherman
Copy link
Contributor Author

unless you are trying to cover the edge case where main.just doesn't exist

Yep, it was just for the paranoid scenario where someone makes their own image without main.just existing anymore, maybe they add a rm command in their own scripts to delete it for example.

Cool. I'm content with being paranoid.

if [ -f ${UBLUE_JUST}/main.just ]; then
cp ${UBLUE_JUST}/main.justfile "${USER_JUSTFILE}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. It's easy to get blind from these filenames since we're writing .just files to .justfile, so I didn't notice that either!

bsherman added a commit to ublue-os/hwe that referenced this pull request May 14, 2023
As of ublue-os/config#42 config repo's
ublue-os-just RPM provides both the main.just and nvidia.just files.
@bsherman
Copy link
Contributor Author

nvidia's related PR is prepped: ublue-os/hwe#101

Change to check if ublue-os-nvidia-addons RPM is installed vs a generic
nvidia RPM as a downstream packager could conceivably use ublue-os/main
and startingpoint, but still roll their own nvidia build and would NOT
want our specific nvidia.just file.
@bsherman bsherman requested review from castrojo and akdev1l May 14, 2023 17:29
@bsherman bsherman marked this pull request as ready for review May 14, 2023 17:29
@@ -7,7 +7,7 @@ if [ ! -z "$HOME" ] && [ -d "$HOME" ] && [ ! -f "${HOME}/.justfile" ]; then
if [ -f ${UBLUE_JUST}/main.just ]; then
cat ${UBLUE_JUST}/main.just >> "${USER_JUSTFILE}"
fi
if [ -f ${UBLUE_JUST}/nvidia.just ] && [ rpm -q xorg-x11-drv-nvidia ]; then
if [ -f ${UBLUE_JUST}/nvidia.just ] && [ rpm -q ublue-os-nvidia-addons ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a really good idea, and good reason for it!

castrojo pushed a commit to ublue-os/hwe that referenced this pull request May 14, 2023
As of ublue-os/config#42 config repo's
ublue-os-just RPM provides both the main.just and nvidia.just files.
@castrojo castrojo merged commit cf3cef8 into main May 14, 2023
@castrojo castrojo deleted the 41-standardize-path-and-behavior-of-ublue-os-justfile branch May 14, 2023 21:43
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.

standardize path and behavior of ublue-os justfile
3 participants