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.

Sign up for GitHub

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

Jump to bottom

Fix incorrect use of bash variable #1797

Merged
merged 2 commits into from
Apr 8, 2021
Merged

Fix incorrect use of bash variable #1797

merged 2 commits into from
Apr 8, 2021

Conversation

jpagex
Copy link
Contributor

@jpagex jpagex commented Apr 8, 2021

Closes: #1796

@monacoremo
Copy link
Member

Thank you jpagex!

Just noticed that we are not running shellcheck on that script in CI. When I run it manually, it raises:

Line 43:
  trap "rm -rf \"$tmpdir\"" sigint sigterm exit
                 ^-- SC2064: Use single quotes, otherwise this expands now rather than when signalled.

Singe quotes would probably work like this: trap 'rm -rf "$tmpdir"' sigint sigterm exit.

If you'd like to get your feet wet in our nix dev environment: To add shellcheck to the postgrest-lint script, you could add shellcheck to the imports at the top of nix/style.nix and add something like the below after line 57:


        # Lint bash scripts
        ${shellcheck}/bin/shellcheck test/with_tmp_db

But don't worry if not, we'll add it separately otherwise :-)

@jpagex
Copy link
Contributor Author

jpagex commented Apr 8, 2021

I changed to single quotes. I wanted to make the change in nix/style.nix but when running nix-shell to test it, I got the following error:

$ nix-shell
error: cannot coerce null to a string, at /Users/jeremy/workspace/postgrest/nix/tests.nix:197:7
(use '--show-trace' to show detailed location information)
$ nix-shell --show-trace
error: while evaluating the attribute 'buildInputs' of the derivation 'ghc-shell-for-postgrest-7.0.1' at /nix/store/1l8fl5m4zzaqkaba6qx3aq29c4kd2fkf-source/pkgs/development/haskell-modules/generic-builder.nix:615:16:
while evaluating the attribute 'passAsFile' of the derivation 'postgrest-devtools' at /nix/store/1l8fl5m4zzaqkaba6qx3aq29c4kd2fkf-source/pkgs/build-support/trivial-builders.nix:7:7:
while evaluating the attribute 'text' of the derivation 'postgrest-check' at /nix/store/1l8fl5m4zzaqkaba6qx3aq29c4kd2fkf-source/pkgs/build-support/trivial-builders.nix:7:7:
while evaluating the attribute 'passAsFile' of the derivation 'postgrest-tests' at /nix/store/1l8fl5m4zzaqkaba6qx3aq29c4kd2fkf-source/pkgs/build-support/trivial-builders.nix:7:7:
while evaluating the attribute 'text' of the derivation 'postgrest-coverage' at /nix/store/1l8fl5m4zzaqkaba6qx3aq29c4kd2fkf-source/pkgs/build-support/trivial-builders.nix:7:7:
cannot coerce null to a string, at /Users/jeremy/workspace/postgrest/nix/tests.nix:197:7

Do you have an idea?

@wolfgangwalther
Copy link
Member 

Line 43:
  trap "rm -rf \"$tmpdir\"" sigint sigterm exit
                 ^-- SC2064: Use single quotes, otherwise this expands now rather than when signalled.

Singe quotes would probably work like this: trap 'rm -rf "$tmpdir"' sigint sigterm exit.

This is one of those cases where I think expanding this "now" is actually better than "when signalled", so the shellcheck advise is not that helpful. Is it possible to disable it for this line?

(In practice it doesn't make that big of a difference, but in theory the $tmpdir variable could be changed later in the script - but we'd still want to clean up the directory that was actually created - so evaluating "right now" seems to be the right thing.)

@jpagex
Copy link
Contributor Author

jpagex commented Apr 8, 2021

Well phrased! That is exactly how I felt about it.

@monacoremo
Copy link
Member

Yes, it doesn't make a difference in our case, and it's easier than to disable the check (which can be done with a comment, see e.g.

postgrest/nix/devtools.nix

Line 121 in 7f11c1a

# shellcheck disable=SC2016
)

I changed to single quotes. I wanted to make the change in nix/style.nix but when running nix-shell to test it, I got the following error:

Not sure what happens there - if you only change nix/style.nix it shouldn't have an impact on the other modules like nix/tests.nix that is mentioned in the trace. Do you have a branch/commit that I can take a look at?

@jpagex
Copy link
Contributor Author

jpagex commented Apr 8, 2021

Not sure what happens there - if you only change nix/style.nix it shouldn't have an impact on the other modules like nix/tests.nix that is mentioned in the trace. Do you have a branch/commit that I can take a look at?

It happens even without adding the code to nix/style.nix. I ran nix-shell and it downloaded a lot of deps. Here is the end of the script call:

Installing executable hackage2nix in /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/bin
Warning: The directory
/nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/bin is not in the
system search path.
Registering library for cabal2nix-2.15.5..
post-installation fixup
strip is /nix/store/i1mccwrczhhacpk0jn58wmixqiqh1vkb-cctools-binutils-darwin-949.0.1/bin/strip
stripping (with command strip and flags -S) in /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib  /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/bin 
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Paths_cabal2nix.hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell.dyn_hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Fetch.dyn_hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal.hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/PackageSourceSpec.hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal/License.hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal/PostProcess.dyn_hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal/Name.hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal/Name.dyn_hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal/Flags.dyn_hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal/Configuration.hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal/License.dyn_hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal/PostProcess.hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal/Configuration.dyn_hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal/Flags.hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal/Normalize.dyn_hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal/Normalize.hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/OrphanInstances.hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/Constraint.dyn_hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/BuildInfo.hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/Derivation.dyn_hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/FromCabal.dyn_hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/Constraint.hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/PackageSourceSpec.dyn_hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc-8.10.2/cabal2nix-2.15.5-5Ds0AyYYKGgJp4WrfQS5CW/Distribution/Nixpkgs/Haskell/BuildInfo.dyn_hi
strip: can't process non-object and non-archive file: /nix/store/39l6b9wjj9shsxgwdc5my4f3v303866r-cabal2nix-2.15.5/lib/ghc-8.10.2/x86_64-osx-ghc

Can it be the cause?

@jpagex
Copy link
Contributor Author

jpagex commented Apr 8, 2021

I had not set up cachix with PostgREST. I will try with it and let you know if it works.

@monacoremo
Copy link
Member

Ok! What kind of machine are you on?

The fix is good as is, I'll merge. If you'd like to contribute the nix/CI enhancement, happy to work in a separate PR!

@monacoremo monacoremo merged commit 67f555d into PostgREST:main Apr 8, 2021
@jpagex
Copy link
Contributor Author

jpagex commented Apr 8, 2021

MacBook Pro 15-inch, 2018
macOS Big Sur (Version 11.2.3)

Thanks for the merge.

@monacoremo
Copy link
Member

Ah, I don't think we've ever tested the nix environment on MacOS, only Linux and in Docker. It would be great if we got it to properly work on MacOS, at least for development without the static compilation stuff.

In the meantime, @wolfgangwalther got the Nix setup to work as is on Windows with Docker: https://github.com/PostgREST/postgrest/blob/main/nix/Dockerfile . That might also work on MacOS?

@wolfgangwalther
Copy link
Member

In the meantime, @wolfgangwalther got the Nix setup to work as is on Windows with Docker: https://github.com/PostgREST/postgrest/blob/main/nix/Dockerfile . That might also work on MacOS?

See here how to start it.

@steve-chavez
Copy link
Member

Strange, the PR made a test fail on CI when merged into master: https://app.circleci.com/pipelines/github/PostgREST/postgrest/850/workflows/1c7c98b4-8081-4239-96a8-fef6a9fb5f1e/jobs/8487

Not sure what happened there.

@wolfgangwalther
Copy link
Member

I'm pretty sure that this is a very rare race-condition that comes from running the test-suite in parallel. We had to clean up a few tests not mutating the database and probably still have some that do manipulate some sequences somehow. I'll add an issue to track and will investigate/fix at some point. I don't think this will come up again immediately.

@wolfgangwalther wolfgangwalther mentioned this pull request Apr 9, 2021
Open
@wolfgangwalther
Copy link
Member

I just retriggered the circle ci workflow and it's passing now - without any changes. So this is really not easily reproducible. I wrote down the link to the error log in the linked issue, though.

@wolfgangwalther
Copy link
Member

wolfgangwalther commented Apr 13, 2021 •
edited
Loading

If you'd like to get your feet wet in our nix dev environment: To add shellcheck to the postgrest-lint script, you could add shellcheck to the imports at the top of nix/style.nix and add something like the below after line 57:

I just hit an edge-case in #1812, where it would be really beneficial to have with_tmp_db completely inside nix as a checked-shell-script itself. Need the version of that script to match with the nix scripts - even when run in another branch. I think we're not running any tests outside of nix anymore, so is there any other reason we can't bring the lost one home? @monacoremo

@monacoremo
Copy link
Member

@wolfgangwalther There is value to having that script separate I think - e.g. for users still using stack only. It's pretty easy to hardwire it into nix though: Simply replace exec test/with_tmp_db "@" with exec ${../test/with_tmp_db} "@" in nix/withtmpdb.nix. That will copy the script to the nix store and will keep it the pinned within a nix-shell session, for example.

@wolfgangwalther
Copy link
Member

Simply replace exec test/with_tmp_db "@" with exec ${../test/with_tmp_db} "@" in nix/withtmpdb.nix. That will copy the script to the nix store and will keep it the pinned within a nix-shell session, for example.

Cool - will do!

@wolfgangwalther wolfgangwalther mentioned this pull request Apr 14, 2021
Merged
6 tasks
monacoremo pushed a commit to monacoremo/postgrest that referenced this pull request Jul 17, 2021
@jpagex
nix: Fix incorrect use of bash variable (PostgREST#1797)
eb0e937
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect use of bash variable
4 participants
@jpagex @monacoremo @wolfgangwalther @steve-chavez