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: quoting of PRJ_PATH/fragment for installables #358

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

kolloch
Copy link

@kolloch kolloch commented Oct 23, 2023

@kolloch
Copy link
Author

kolloch commented Oct 23, 2023

I also enabled showing the actual command with set -x that might not be to your test.

The precommit hooks in the devshell also put the updated flake.locks into this commit. Not sure about that.

@whs-dot-hk
Copy link
Contributor

@kolloch
Copy link
Author

kolloch commented Oct 24, 2023

@whs-dot-hk I am not quite sure what you are getting it.

Maybe somemore context on what I did with the fragment:

I am quoting the fragment for shell usage. That would actually mean that the string including the double quotes is passed to the nix command. Right now they are interpreted by bash, so that the string passed to the command does not contain the quotes anymore. That would make a difference, if anything in the path would contain spaces or other special characters, otherwise not so much. Still, I think it is generally good practice to quote things properly so that you don't even have to think about corner cases.

@whs-dot-hk
Copy link
Contributor

I was trying to understand what is fragment, it seems that it means a flake fragment

I am not sure where to escape, but I think yours is a good place

Do other places in the code also need to change?

@kolloch
Copy link
Author

kolloch commented Oct 25, 2023

Do other places in the code also need to change?

Hard to say.

❯ rg PRJ_ROOT
src/std/fwlib/_mkCommand.nix
27:          if test -z "$PRJ_ROOT"; then

src/std/fwlib/blockTypes/terra.nix
69:        dir="$PRJ_ROOT/${repoFolder}/.tf"
70:        mkdir -p "$PRJ_ROOT/${repoFolder}/.tf"
71:        cat << MESSAGE > "$PRJ_ROOT/${repoFolder}/.tf/readme.md"

src/std/fwlib/blockTypes/installables.nix
37:        nix profile install $PRJ_ROOT#${fragment}
41:        nix profile upgrade $PRJ_ROOT#${fragment}
45:        nix profile remove $PRJ_ROOT#${fragment}
50:        nix bundle --bundler github:Ninlives/relocatable.nix --refresh $PRJ_ROOT#${fragment}
54:        nix bundle --bundler github:NixOS/bundlers#toDockerImage --refresh $PRJ_ROOT#${fragment}
58:        nix bundle --bundler github:ralismark/nix-appimage --refresh $PRJ_ROOT#${fragment}

PRJ_ROOT itself looks good to me.

Otherwise one would need to go through all instances in which nix variables are injected into shell scripts (that are not just store paths) and escape them.

Copy link
Collaborator

@blaggacao blaggacao left a comment

Choose a reason for hiding this comment

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

That looks pretty sensible, indeed!

Let's wait for the test run.

The flake lock update is an ugly attempt for sub- / superflake compliance untill subflakes get better upstream support.

@blaggacao blaggacao merged commit 403fd2a into divnix:main Nov 3, 2023
5 checks passed
@kolloch kolloch deleted the fix/357-quoting branch November 4, 2023 10:03
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