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

fix: Split justfiles into multiple numbered files to fix issue with default command on other releases. #118

Merged
merged 14 commits into from
Sep 22, 2023

Conversation

KyleGospo
Copy link
Member

Cleaner thanks to commands now being categorized, and additionally fixes an issue where just files are loaded alphabetically and the default command can change as new files are added.

Brings in comments and listing improvements made at Bazzite.

@KyleGospo KyleGospo requested a review from castrojo September 20, 2023 07:19
@castrojo castrojo marked this pull request as draft September 20, 2023 20:47
@castrojo
Copy link
Member

cc @ublue-os/approver for feedback.

I think we need to account for recreating the existing user's ~/.justfile so that they have a migration path. Probably a one shot service unit?

@KyleGospo
Copy link
Member Author

Alternative option, do we have the ability to define what's in ~/.justfile in a system level file and avoid having to use files in home at all?

@p5
Copy link
Member

p5 commented Sep 21, 2023

Alternative option, do we have the ability to define what's in ~/.justfile in a system level file and avoid having to use files in home at all?

For my image, I have aliased eternal (for Eternal OS 😄) to be just --unstable --justfile /usr/share/eternal/justfile.
So by default, users (just me) needs to run eternal and uses the justfiles installed in the system.

The /usr/share/eternal/justfile file contains only includes for other justfiles which are added onto each image during build -

RUN echo "!include /usr/share/eternal/lumina.just" >> /usr/share/eternal/justfile && \
  echo "!include /usr/share/eternal/lumina-setup.just" >> /usr/share/eternal/justfile

This gets around the requirement for justfiles to be installed at a user level.

RE bsherman's comment about Nvidia, you could simply copy over the justfile for all images, and add the include only during Nvidia builds.

Just an idea... But there could be a ublue alias

Copy link
Contributor

@bsherman bsherman 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 requesting these changes, but let me know if I'm off base.

build/ublue-os-just/40-nvidia.just Show resolved Hide resolved
@joefiorini
Copy link

I like @p5's idea of using an alias, except that might require a little more effort to be supported across different shells. is there appetite for having a fairly simple cli that wraps just and sets up paths on its own?

In the meantime, what about having a master justfile in /usr/share/ublue-os that includes all of the numbered files in whatever order you want? Then in ~/.justfile it becomes:

...
!include /usr/share/ublue-os/master.justfile
...

You still have to deal with the user being able to edit ~/.justfile, but its a start 😉

@KyleGospo
Copy link
Member Author

I like @p5's idea of using an alias, except that might require a little more effort to be supported across different shells. is there appetite for having a fairly simple cli that wraps just and sets up paths on its own?

In the meantime, what about having a master justfile in /usr/share/ublue-os that includes all of the numbered files in whatever order you want? Then in ~/.justfile it becomes:

...
!include /usr/share/ublue-os/master.justfile
...

You still have to deal with the user being able to edit ~/.justfile, but its a start 😉

Latest commit does the master justfile as /usr/share/ublue-os/justfile, now we just need the .justfile editor as mentioned

…o new /usr/share/ublue-os/justfile

fixme: Modify existing .justfiles to have this same change in a non-destructive way.
…ve user-added lines.

chore: Minor spec cleanup
@KyleGospo KyleGospo marked this pull request as ready for review September 22, 2023 00:42
feat: Add installer for obs-studio-portable
@KyleGospo
Copy link
Member Author

This now depends on this landing first:
ublue-os/main#350

Copy link
Member

@EyeCantCU EyeCantCU left a comment

Choose a reason for hiding this comment

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

Tested, can confirm things are working as intended!

@KyleGospo KyleGospo enabled auto-merge September 22, 2023 22:52
@KyleGospo KyleGospo requested a review from bsherman September 22, 2023 23:31
Copy link
Contributor

@bsherman bsherman 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! Good improvements here!

@KyleGospo KyleGospo added this pull request to the merge queue Sep 22, 2023
Merged via the queue into main with commit b60bd12 Sep 22, 2023
1 check passed
@KyleGospo KyleGospo deleted the just-cleanup branch September 22, 2023 23:48
castrojo pushed a commit to ublue-os/beyond that referenced this pull request Sep 23, 2023
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.

6 participants