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

Ensure write permissions before and after lang file update #824

Merged
merged 1 commit into from
Jun 7, 2023

Conversation

garaiza-93
Copy link
Contributor

@garaiza-93 garaiza-93 commented Jun 7, 2023

Related to #822

Use chmod to make sure language files in the user's config directory have write permissions before AND after updating them.

I've decided on ensuring write permissions before and after the update just in case of an unforeseen edge case.

Testing

  1. Remove write permissions from all language files in the config's lang/ directory with chmod -R -w /path/to/config/lang
  2. In a terminal, execute steam
  3. Pick a game. In the game's Properties, force the use of steamtinkerlaunch
  4. Start the game
  5. Check that in steam's stdout, there are no complaints of rm waiting for user input to proceed with removing existing language file(s)

@garaiza-93 garaiza-93 changed the title ensure write permissions before and after lang file update Ensure write permissions before and after lang file update Jun 7, 2023
@sonic2kk
Copy link
Owner

sonic2kk commented Jun 7, 2023

Wow! Thank you 😄

I've decided on ensuring write permissions before and after the update just in case of an unforeseen edge case.

I had a stashed change that did exactly this (stashed because I was working on d57e572 😉), so I think this is a sensible approach. I didn't have time to test it so I appreciate the work you've done here!

Check that in steam's stdout, there are no complaints of rm waiting for user input to proceed with removing existing language file(s)

Ah, duh, I'm dumb. In #822 I wondered why this was causing issues, but it was even mentioned in the NixOS issue that rm waiting on user input causing the problem 😅


There are two very small things I would just add before merging:

  1. I wonder if it's best to do this before if [ -f "$LANGFILENAME" ]; then (line 2236 in this PR), and then again at the very bottom of the if block that encapsulates the whole method (line 2269 in this PR). It probably doesn't matter since this issue should pretty much only affect Nix, so I will leave it to your discretion (if it turns out to be useful in other cases it can addressed at that point). I'm happy to merge it as-is :-)
  2. The more pertinent question: How do we want to handle getting this into the Nix package? This will be merged into master, the """release strategy""" for STL is to basically merge to master and then, for lack of a better phrase, "once I feel like it" (and when I have a cool name in mind!) I make a tag and release from all the commits in master since the last release. That doesn't bode too well for Nix, where this fix is kind of... urgent. It might be possible to take this PR as a patch and apply it to the Nix package at install time. Of course, either way, this won't impact it getting merged, because it's a correct fix, just wondering what would come after :-)

Awesome work here! Thanks for taking the time to make this change and test it, this will be ready to merge after some feedback on the above questions :-)

@sonic2kk
Copy link
Owner

sonic2kk commented Jun 7, 2023

Oh, I was just thinking, if you think the second point about how to get this into Nix would be better discussed in the NixOS/nixpkgs#226086 I can chime in there :-)

@garaiza-93
Copy link
Contributor Author

I wonder if it's best to do this before if [ -f "$LANGFILENAME" ]; then (line 2236 in this PR), and then again at the very bottom of the if block that encapsulates the whole method (line 2269 in this PR).

I think it's fine as-is for readability, instead of having to look at a larger scope.

The more pertinent question: How do we want to handle getting this into the Nix package?

Not a problem! It's just a matter of telling Nix to look for this commit hash. so we don't have to wait for the next release! However, if the maintainer for STL's package wants to do things differently, that's up to them.

For cool edu-ma-cational purposes, here's how Nix gets STL's code:

  src = fetchFromGitHub {
    owner = "sonic2kk";
    repo = pname;
    rev = "v${version}";
    hash = "sha256-oigHNfg5rHxRabwUs66ye+chJzivmCIw8mg/GaJLPkg=";
  };

Here, pname is just steamtinkerlaunch, just stored in a variable. owner and repo are basic repo information, as in who owns it and the name of the repository. rev is the cool part though, because instead of a tag, it can just as easily be a commit hash and it'll fetch the code no problem! hash is used internally to identify the directory extracted from the source archive. If you're curious, all the packaging for STL is here!

@sonic2kk
Copy link
Owner

sonic2kk commented Jun 7, 2023

I think it's fine as-is for readability, instead of having to look at a larger scope.

Sounds good to me :-)

Not a problem! It's just a matter of telling Nix to look for this commit hash. so we don't have to wait for the next release! However, if the maintainer for STL's package wants to do things differently, that's up to them.

That is fair enough! They are always free to reach out to me if they need any assistance as well (not sure if I can provide any 😅)

For cool edu-ma-cational purposes, here's how Nix gets STL's code:

That is pretty sweet and very readable actually (also, still get a kick out of seeing sonic2kk in other people's code heh). I wonder if pointing to a specific revision would cause all the changes from 12.12 up to now to be pulled in... But either way, like you said, that's probably up to the maintainer to figure out.


This looks good to me, ready to merge I think 👍

Thanks!

@sonic2kk sonic2kk merged commit bde0812 into sonic2kk:master Jun 7, 2023
@garaiza-93 garaiza-93 deleted the ensure-write-permissions branch June 7, 2023 20:49
@sonic2kk
Copy link
Owner

sonic2kk commented Jun 7, 2023

D'oh, forgot to ask you to bump the version, sorry about that (it's not a big deal, I forget fairly often to do it myself). Going to push a small commit to bump it.

Also, updated the changelog to include mention of your fix, so when a release comes out and I copypaste these release notes you'll probably get pinged :-)

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.

2 participants