Skip to content

move-docs.sh: shellcheck 2166#130595

Merged
happysalada merged 2 commits intoNixOS:stagingfrom
happysalada:move_docs_shellcheck_2166
Aug 30, 2021
Merged

move-docs.sh: shellcheck 2166#130595
happysalada merged 2 commits intoNixOS:stagingfrom
happysalada:move_docs_shellcheck_2166

Conversation

@happysalada
Copy link
Copy Markdown
Contributor

@happysalada happysalada commented Jul 19, 2021 •
edited
Loading

Motivation for this change

This PR is in the series of stdenv shell improvements
commit from a reject PR commit a37fa9e

This is coming straight from the book shellcheck 2166
https://github.com/koalaman/shellcheck/wiki/SC2166

IMO it should be straightforward

I'm grouping this with another commit ebf3f62
Since those are very similar

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Relase notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@happysalada happysalada requested a review from Ericson2314 as a code owner July 19, 2021 03:00
@happysalada happysalada mentioned this pull request Jul 19, 2021
Merged
11 tasks
@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Jul 19, 2021
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jul 19, 2021
@SuperSandro2000
Copy link
Copy Markdown
Member

SuperSandro2000 commented Jul 20, 2021 •
edited
Loading

please target staging

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 20, 2021

Thank you for your review!
I didn't mention it in every single PR but my idea was to try to get faster builds to verify that my changes at least build.
Once I have received feedback on the changes and it's a good idea to merge them, I will target staging. Before a merge, somebody for a hydra team signified that they want me to ask them to verify that the hydra build is good. The last stream of changes introduced regressions.

@SuperSandro2000
Copy link
Copy Markdown
Member

SuperSandro2000 commented Jul 20, 2021 •
edited
Loading

I didn't mention it in every single PR but my idea was to try to get faster builds to verify that my changes at least build.

You are rebuilding stdenv everytime. It shouldn't matter which branch you are targeting. Both master and staging will take some time to build. I think the only difference is the deps for the stdenv.

@happysalada
Copy link
Copy Markdown
Contributor Author

happysalada commented Jul 21, 2021

I could be wrong here. It seems to me that the jobs targeting staging have a lower priority in the queue than the jobs targeting master. On another PR I am requesting the help from someone in the hydra team. I will make sure to confirm with them.
(In the previous PR I made, when targetting staging I remember it taking longer for the tests to be run, it might have been a coincidence).

@happysalada happysalada changed the base branch from master to staging July 23, 2021 12:53
@happysalada happysalada mentioned this pull request Aug 3, 2021
Closed
11 tasks
@happysalada happysalada force-pushed the move_docs_shellcheck_2166 branch from 4dc919e to 7fb0fb1 Compare August 18, 2021 15:55
7c6f434c
7c6f434c reviewed Aug 18, 2021
View reviewed changes
pkgs/stdenv/generic/setup.sh Outdated
Copy link
Copy Markdown
Member

@7c6f434c 7c6f434c Aug 18, 2021

Choose a reason for hiding this comment

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

Does it count as SC 2166 if the code you start with is already what shellcheck considers correct?

Copy link
Copy Markdown
Contributor Author

@happysalada happysalada Aug 18, 2021

Choose a reason for hiding this comment

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

You're right here. This was part of something else. Happy to revert.

Copy link
Copy Markdown
Member

@7c6f434c 7c6f434c Aug 19, 2021

Choose a reason for hiding this comment

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

Not sure about something else, there are two more changes like this in this PR… (and one change that got converted from a true SC 2166 to a logic simplification)

Copy link
Copy Markdown
Contributor Author

@happysalada happysalada Aug 19, 2021

Choose a reason for hiding this comment

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

Yes, I would like to bring in the logic simplification part, I don't mind renaming the commit if you want.
I don't mind removing the [[ -> [.
Let me know what you think should be removed and I can have a look.

Copy link
Copy Markdown
Member

@7c6f434c 7c6f434c Aug 19, 2021

Choose a reason for hiding this comment

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

I would expect logic simplification to be in a commit mentioning logic simplification, and [ to [[ conversion can happen, but then it should be called something about conditional style. I just don't think like having a commit called SC 2166 doing two different things, of which neither is actually a fix for SC 2166, is a good idea.

(Having a single commit with a long commit message is also fine — I just want to avoid an actively misleading commit title)

Copy link
Copy Markdown
Contributor Author

@happysalada happysalada Aug 19, 2021

Choose a reason for hiding this comment

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

Let me amend the commit message then.

Copy link
Copy Markdown
Contributor Author

@happysalada happysalada Aug 19, 2021

Choose a reason for hiding this comment

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

I've tried to be more descriptive, let me know if you still think it needs work.

Copy link
Copy Markdown
Member

@7c6f434c 7c6f434c Aug 19, 2021

Choose a reason for hiding this comment

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

The original commit now looks a bit pointless… The rest is good, in my opinion

Copy link
Copy Markdown
Contributor Author

@happysalada happysalada Aug 20, 2021

Choose a reason for hiding this comment

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

yeah I missed it. Let me just squash those.

@happysalada happysalada force-pushed the move_docs_shellcheck_2166 branch from 7fb0fb1 to f23c4e0 Compare August 19, 2021 14:24
@happysalada
move-docs.sh: simplify redundant conditional
8ca4655 
remove impossible check for empty variable after it's been set twice
@happysalada
stdenv setup.sh: remove combined [ in favor of [[
bb482d5 
[ ... ] && [ ...] -> [[ ... && ... ]]
@happysalada happysalada force-pushed the move_docs_shellcheck_2166 branch from f23c4e0 to bb482d5 Compare August 20, 2021 08:06
@happysalada happysalada merged commit e32bf6f into NixOS:staging Aug 30, 2021
@happysalada happysalada deleted the move_docs_shellcheck_2166 branch August 30, 2021 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@7c6f434c 7c6f434c 7c6f434c approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

Assignees

No one assigned

Labels

6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@happysalada @SuperSandro2000 @7c6f434c