-
Notifications
You must be signed in to change notification settings - Fork 12
Move dep-vars - make wheel available for release #455
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
Conversation
kmontemayor2-sc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I'm not thrilled about having the scala binaries having some dependency on the python src dirs, seems "incorrect" somehow...
I guess the alternative here is to duplicate dep_vars.env directly in constants.py when we build it? tbch I sort of prefer that so the values are obvious when clicking around python src dirs (instead of needing to then go find the dep_vars.env file).
WDYT?
Otherwise this is fine.
The build not mirroring src seems worse to me. |
|
It would mirror src, I'm suggesting that our scripts which generate |
kmontemayor2-sc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approval pending we've check the release paths work.
Yep, tested here: https://github.com/Snapchat/GiGL/actions/runs/21468608715, and publish wheel here: https://github.com/Snapchat/GiGL/actions/runs/21469349436 |
Scope of work done
Without this change
dep_vars.envis not available in the released wheel, as such anything that relies ongigl/common/constants.pydoes not work since that requiresdep_vars.envbeing avaialble.We move the file under the
gigldir and also make all yaml files in these dirs also package in the wheels to ensure the build mirrors the source layout and support bootstrapping.Where is the documentation for this feature?: N/A
Did you add automated tests or write a test plan?
Updated Changelog.md? NO
Ready for code review?: YES