Page MenuHomePhabricator

[Nix] Use file to determine user preference
ClosedPublic

Authored by jon on Aug 15 2022, 6:49 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jun 13, 2:34 PM
Unknown Object (File)
Wed, Jun 5, 12:05 AM
Unknown Object (File)
Thu, May 23, 12:04 AM
Unknown Object (File)
Sat, May 18, 6:08 AM
Unknown Object (File)
May 15 2024, 3:06 AM
Unknown Object (File)
Apr 20 2024, 6:12 PM
Unknown Object (File)
Apr 20 2024, 6:12 PM
Unknown Object (File)
Apr 20 2024, 6:11 PM

Details

Reviewers
atul
varun
abosh
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rCOMM04916a6cd2ae: [Nix] Use file to determine user preference
Summary

Avoid looking at PS1 to determine user's preference,
and just explicity ask them.

Use a file to save the preference between invocations (e.g. nix develop)

Test Plan
nix develop
# should get prompted
# if yes, prompt should change
# if no, prompt should not change

rm ~/Library/Caches/app.comm/enable-powerline
exit

nix develop
# try other option

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jon edited the test plan for this revision. (Show Details)
ashoat added inline comments.
nix/start-powerline.sh
16 ↗(On Diff #15633)

Should Powerline be capitalized?

Going to review this more carefully after weekly sync, but just wanted to make sure these changes are solely cosmetic?

Doesn't it make the nix environment a little less "deterministic" in that it introduces potential variability for different members of the team? Not sure it makes a practical difference but just wondering.

Nix is about reproducable builds, not about forcing every dev to have the exact same settings for their shell

Nix is about reproducable builds, not about forcing every dev to have the exact same settings for their shell

Couldn't different shell settings potentially break reproducibility?

Like if one dev sets a shell option eg

shopt -s globstar

that would definitely change behavior of build scripts and break reproducibility.

These changes definitely seem "benign" and cosmetic and I'm not arguing against them, just figured we'd generally want to limit the amount of variability in environment.

Couldn't different shell settings potentially break reproducibility?

This is true, but even if we didn't include the option to use Powerline it would still be true. Even if we're all on Nix, we can't prevent someone from changing their settings. Developers will inevitably have different shell settings or use different shells altogether. I myself use fish which is different from zsh and bash, but it's my responsibility to understand the differences and take into account that variability.

Your point is still valid. The globstar example is a good one because using ** in scripts is really common because it's so useful. Yet it can only be run if the globstar option is set. Thus in the future, we may run into issues with different shell configurations, but this script's purpose is just to change the prompt / colors a user sees. It shouldn't modify something that could break reproducibility.

@jon I'm curious for your input.

nix/start-powerline.sh
15 ↗(On Diff #15633)

I've seen this before in read commands, but why do we need to prevent backlashes here? I know a user could provide malformed or malicious input (that could contain backslashes) but we wouldn't match it anyways / do anything with the command based on our case statement, right? Or is adding -r just standard for read?

16 ↗(On Diff #15633)

Can we hyperlink Powerline? Just Googling "Powerline" does not provide any immediately meaningful results (no Wikipedia sidebar, top result is not Powerline docs/website). More information is always better if we're modifying user experience.

18–25 ↗(On Diff #15633)

case statements can be difficult to read. What version of bash are we using? If we're on version 3.2+, we can use an if-else:

if [[ "$response" =~ ^([yY][eE][sS]|[yY])$ ]]
then
    echo "1" > "${COMM_POWERLINE}"
else
    touch "${COMM_POWERLINE}"
fi

If we're on bash 4.0+:

response=${response,,} # Convert the response to lowercase
if [[ "$response" =~ ^(yes|y)$ ]]
then
    echo "1" > "${COMM_POWERLINE}"
else
    touch "${COMM_POWERLINE}"
fi
  • Read more about using if-else for matching y/n
  • Read more about converting string to lowercase in bash
28 ↗(On Diff #15633)

Could we just use -s here?

From https://www.gnu.org/software/bash/manual/bash.html:

-s file
True if file exists and has a size greater than zero.

This revision now requires changes to proceed.Aug 16 2022, 9:42 AM

Couldn't different shell settings potentially break reproducibility?

Nix has strong guarantees around building packages with it. Shell usage is a non-goal for nix.

Like if one dev sets a shell option eg

We've already had this with zsh's extGlob syntax and that things like nix run .#mariadb-up, the solution is to escape it nix run '.#mariadb-up'.
After nix develop is done, this is a non-issue because we are now in bash, but it's still odd.

that would definitely change behavior of build scripts and break reproducibility.

Not in a meaningful way, the shebangs will force the scripts to be executed by a particular shell. Currently we force bash on almost all of our scripts anyway.

These changes definitely seem "benign" and cosmetic and I'm not arguing against them, just figured we'd generally want to limit the amount of variability in environment.

Yea, but I would argue there's a difference between how a developer interfaces with tooling, and what tooling is present. The main purpose of leveraging nix is so that everyone has the same tooling, but I think it's best for developers to choose their own way to interface with it.

For example, I already have a pretty extensive bash setup, and would like to have it over powerline in my setups.

jon added inline comments.
nix/start-powerline.sh
15 ↗(On Diff #15633)

I believe it's considered best practice, but every invocation of read I see includes it. So I think we should keep -r.

16 ↗(On Diff #15633)

Yes, we should.

16 ↗(On Diff #15633)

Can we hyperlink Powerline?

I would like to avoid this, AFAIK, hyperlink support for terminals is very hit or miss on a given terminal. And I'm not aware of any standard way to do it.

I would be find with just adding another line which has the full explicit link to the github readme, my terminal emulator is at least able to auto-hyperlink valid hyperlinks.

Maybe punt on this and add it later, as I think it increases the scope of what is trying to be achieved here; and this is blocking other work like the binary cache. I created https://linear.app/comm/issue/ENG-1627 to track this.

18–25 ↗(On Diff #15633)

The intention is that this can also be done with zsh or ancient versions of bash. We can't make assumptions about the shell until after nix develop, and even then would still like to avoid shell-specific patterns.

case, although awkward, is pretty universally supported.

28 ↗(On Diff #15633)

Yea, this would probably be better.

jon marked 5 inline comments as done.

Use -s for checking if preference file exists

Ok, thanks for addressing my feedback!

This revision is now accepted and ready to land.Aug 16 2022, 10:28 AM

Even if we're all on Nix, we can't prevent someone from changing their settings. Developers will inevitably have different shell settings or use different shells altogether. I myself use fish which is different from zsh and bash, but it's my responsibility to understand the differences and take into account that variability.

You're able to use fish in a nix environment?

You're able to use fish in a nix environment?

You would have to manually launch it after nix develop but you can. fish will inherit all of the relevant ENV vars needed.

Does this stack fix the issue I was seeing where press-up-to-see-previous-commands wasn't working?

Does this stack fix the issue I was seeing where press-up-to-see-previous-commands wasn't working?

I think that's a separate issue, as my terminal has the normal "up means go up in history" behavior.

In https://linear.app/comm/issue/ENG-1550#comment-854f3150 I mention that it's possible that your terminal is somehow put in "alternate console" mode. The fix for which is usually doing tput rmcup. I could forcefully run this in nix develop as this will likely have the desired behavior anyway, but might cause issues with things like tmux (haven't tried).

But how did my terminal ever get into the "alternate console" mode to begin with?

Pretty much any application which grabs the cursor will do this: less, more, vim, nano, etc. If the program doesn't exit cleanly then your console can be in this weird state of not being "released" back to the parent process correctly. If you're only getting this when doing nix develop, then that's very odd. My m1, intel mac and NixOS aren't having that behavior even when using Powerline. We can create another task, and I can try to troubleshoot it separately.

Owners added a reviewer: Restricted Owners Package.Aug 16 2022, 2:17 PM