Page MenuHomePhabricator

[keyserver] Use `[[ ]]` instead of `[ ]` in `source-nvm.sh`
ClosedPublic

Authored by abosh on Jun 30 2022, 2:02 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 13, 4:40 AM
Unknown Object (File)
Tue, Nov 5, 8:10 PM
Unknown Object (File)
Tue, Nov 5, 8:10 PM
Unknown Object (File)
Tue, Nov 5, 8:10 PM
Unknown Object (File)
Tue, Nov 5, 8:10 PM
Unknown Object (File)
Tue, Nov 5, 8:10 PM
Unknown Object (File)
Tue, Nov 5, 8:06 PM
Unknown Object (File)
Mon, Oct 28, 6:17 AM
Subscribers

Details

Summary

Relevant Linear issue here. Replaced [ ] with [[ ]] in source-nvm.sh.

Depends on D4429

Test Plan

N/A

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 30 2022, 2:04 PM
Harbormaster failed remote builds in B10179: Diff 14054!
abosh requested review of this revision.EditedJul 1 2022, 7:47 AM

This is running into some errors, and I think it has to do with the difference between sh and bash.

After viewing the logs on Buildkite for the failed Docker keyserver build, I noticed that source_nvm.sh is being interpreted by sh, not bash (view the highlighted portion):

image.png (474×1 px, 141 KB)

However, the shebang in source_nvm.sh says to interpret the script using bash:

image.png (170×560 px, 26 KB)

(Also see: D3784)

This usually isn't an issue, but since bash is a superset of sh there's technically some things that can be run in a bash script that won't be interpreted by sh. One of those being [[ in conditional statements. This is because sh is POSIX-compliant "and no more," and thus [ is executable while [[ is not.

Anyways, I tested the Docker keyserver build locally after changing some things and it still didn't work. It cannot recognize the [[. As soon as I change it back to [, however, the keyserver builds with no errors.

Not sure why source_nvm.sh is being interpreted by sh since the shebang is for bash.

Read more about [ vs. [[ in sh and bash:

atul requested changes to this revision.Jul 1 2022, 4:17 PM

Passing back until it passes CI (discussed the issue and possible solution offline)

This revision now requires changes to proceed.Jul 1 2022, 4:17 PM

(@yayabosh I think you'll need to rebase on top of D4429 and then run arc diff again to fix CI. Just setting the dependency won't work)

Yup, I understand. I only added the dependency to connect the two diffs since that diff must be landed for this one to work.

Retry Docker keyserver build

keyserver/bash/source-nvm.sh
9 ↗(On Diff #14122)

After rebasing on top of D4421, we should convert this usage too

I think I figured out what was breaking this. After inspecting the errors on the Docker keyserver builds, it seems that source-nvm.sh was still being executed by sh instead of bash, even though we had updated the keyserver Dockerfile in D4429. The issue was that there were other places in the codebase that were still sourcing source-nvm.sh in the current environment as opposed to running it in bash (as the shebang in source-nvm.sh indicates).

Ran a quick find-all search for . source-nvm.sh usages instead of ./source-nvm.sh (the former is sourcing the script to run in the current environment, the latter is executing it in bash), and found 3 instances in package.json. Changed these to ./ and now the Docker keyserver builds with no errors:

Screen Shot 2022-07-05 at 2.33.34 PM.png (1×1 px, 479 KB)

abosh added inline comments.
keyserver/package.json
10 ↗(On Diff #14198)

The changes made in this file are explained in more detail in the comment above.

This revision is now accepted and ready to land.Jul 5 2022, 12:22 PM

Switching from . bash/source-nvm.sh to ./bash/source-nvm.sh is causing problems. See this recording of a meeting with Albert starting @5min... doing ./bash/source-nvm.sh results in the wrong Node being used, which leads to a crash.

@ashoat I don't think the recording is loading? I'm getting a 404 Not Found:

image.png (2×3 px, 278 KB)