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

fix(silo-preprocessing): Fix interpolation syntax error #2855

Merged

Conversation

tsibley
Copy link
Contributor

@tsibley tsibley commented Sep 20, 2024

Accidentally introduced in "feat(silo-prepro): use hash instead of line count to test for equality (#1488)" (b0523ad) when changing the interpolation.

Accidentally introduced in "feat(silo-prepro): use hash instead of line
count to test for equality (loculus-project#1488)" (b0523ad) when changing the
interpolation.
@corneliusroemer
Copy link
Contributor

Thanks @tsibley!

@theosanderson theosanderson changed the base branch from main to interpolation-syntax September 22, 2024 17:19
@theosanderson theosanderson merged commit 977049b into loculus-project:interpolation-syntax Sep 22, 2024
4 of 15 checks passed
@theosanderson
Copy link
Member

I transferred this commit to #2856 as I'm not sure we have a way to run our CI here

@corneliusroemer
Copy link
Contributor

@tsibley I'm curious how you came across this bug - were you reading through the code out of curiosity or did you notice something misbehaving in a test environment you'd set up?

It looks like if we hit this branch previously bash would throw a syntax error instead of removing the directory as intended so I'm wondering why we hadn't caught the bug earlier.

@tsibley
Copy link
Contributor Author

tsibley commented Sep 23, 2024

@corneliusroemer I figured you'd be curious. :-) I was reading thru parts of the codebase (on behalf of the Blab Nextstrain team) to understand if/how Loculus could be useful for Nextstrain's curated pathogen data (e.g. to present that data more approachably).

I believe if you hit this branch previously it wouldn't actually throw any error—bash just sees an interpolation ($old_input_data_dir) followed by a few literal characters (:?}) and the -f suppresses rm's complaints via exit status about the path not existing—but instead it would just quietly fail to remove the directory as intended. This kind of error isn't something ShellCheck catches (I checked), but it'd be nice if it did!

@tsibley
Copy link
Contributor Author

tsibley commented Sep 23, 2024

(In retrospect, I probably should have written that second paragraph for the commit message as the analysis of a bug's ramification is useful commit context.)

@corneliusroemer
Copy link
Contributor

Strange thing is I would have thought we'd catch this kind of failure to remove a directory - but apparently we didn't? Anyways, good that you caught it. We've been discussing rewriting that shell script in a proper language with better linters but haven't got there yet.

@fengelniederhammer here we have an example of bash causing a bug as you predicted.

@tsibley
Copy link
Contributor Author

tsibley commented Sep 25, 2024

@chaoran-chen FYI, this same bug exists in the copy of silo_import_job.sh in GenSpectrum/loculus-config.

@chaoran-chen
Copy link
Member

@tsibley, thanks for pointing it out!

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.

4 participants