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

Generate init.sh in one place #819

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Conversation

TimoWilken
Copy link
Contributor

This reduces the duplication in the init.sh generation code significantly.

Instead of generating shell commands that generate init.sh, scattered all over the place, generate the contents of init.sh in one place and just write them into the file when applicable.

Once created, load the generated file instead of generating the same shell code twice at different escaping levels.

All of this reduces the potential for errors a lot, since different places in the code don't need to be kept in sync when generating and re-generating the file.

Add tests that make sure the generated file contents are vaguely sensible, and that clobbering the file during the build (as the AliEn-Runtime package does) does not mean it is mangled during subsequent builds.

This PR requires #818 for the added tox test to work, so I've included that commit here. I'll rebase once that PR is merged.

@TimoWilken TimoWilken requested a review from ktf December 7, 2023 15:22
@TimoWilken TimoWilken force-pushed the extract-init.sh branch 2 times, most recently from 6201ffa to 7178ce5 Compare December 11, 2023 09:52
This reduces the duplication in the init.sh generation code significantly.

Instead of generating shell commands that generate init.sh, scattered all over
the place, generate the contents of init.sh in one place and just write them
into the file when applicable.

Once created, load the generated file instead of generating the same shell
code twice at different escaping levels.

All of this reduces the potential for errors a lot, since different places in
the code don't need to be kept in sync when generating and re-generating the
file.

Add tests that make sure the generated file contents are vaguely sensible, and
that clobbering the file during the build (as the AliEn-Runtime package does)
does not mean it is mangled during subsequent builds.
@TimoWilken
Copy link
Contributor Author

Rebased and fixed another issue found by @pzhristov in AliRoot-OCDB. Ready to review/merge now for me.

@TimoWilken TimoWilken marked this pull request as ready for review December 11, 2023 10:04
@ktf
Copy link
Member

ktf commented Dec 11, 2023

The feature itself is fine for me. My only worry is that the mangling at:

https://github.com/alisw/alibuild/pull/819/files#diff-44e897471db949fd0707d8ab0961b950f94569114948a039d69abaec52b63d7cL83

is not accounted correctly, however I guess we can simply merge this and try it for a bit before we tag it for production.

@ktf ktf merged commit c2f15c4 into alisw:master Dec 11, 2023
11 checks passed
@TimoWilken TimoWilken deleted the extract-init.sh branch December 11, 2023 12:24
@TimoWilken
Copy link
Contributor Author

The feature itself is fine for me. My only worry is that the mangling at:

https://github.com/alisw/alibuild/pull/819/files#diff-44e897471db949fd0707d8ab0961b950f94569114948a039d69abaec52b63d7cL83

is not accounted correctly, however I guess we can simply merge this and try it for a bit before we tag it for production.

Ah, I changed that to a proper json.dumps call, since json has been built-in to the Python standard library for a long time, and the original way of stringifying a Python dict and replacing the quotes seemed really fragile to me.

With #815, that'll be replaced anyway though...

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