-
Notifications
You must be signed in to change notification settings - Fork 1k
fix: re-add variant support for templates #6489
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks @dankm. Please sign the CLA when you get a chance. Also, why is this needed? Does detection not work for some distro? |
I did earlier, but after I submitted this pull request. I had previously signed, but seems the terms were updated.
Detection works everywhere I've tried. My use case is cross-compiling. In particular in OpenEmbedded. We want the cloud-init it builds to reflect the target system, not whatever host system happens to be doing the building. Without this change I get different output on a Ubuntu host and a Fedora host, despite my target system being neither. |
Restore the previous support for specifying the variant used in template rendering instead of always depending on the host. Before canonical#5027 this was configured with the --distro option to setup.py, so use the distro meson option. Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca>
a4ad911 to
aeb0570
Compare
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.
Thanks for this proposal @dankm. One comment, plus there is a merge conflict.
@blackboxsw Supporting two ways to accomplish the same thing is a smell. If we're going to support cross-compiling, then detection logic isn't a source of truth that we can reliably trust - and it is logic that we would have to maintain in upstream indefinitely for N distros. In other words, it is low value and bears a cost. The meson build tooling is still new and will have to be adopted by every distro eventually (I doubt many, if any, have). I suggest that instead of using "detection with overrides", we could just require it to always be set. This would give us a single source of truth we can always trust. Anybody that has already added meson support will have to add the extra argument, but I doubt there are many of those and the change is not significant. For everyone else it will just be part of the transition. Thoughts?
| @@ -1,4 +1,5 @@ | |||
| option('init_system', type: 'string', value: 'systemd', description: 'Set target init system.') | |||
| option('distro', type: 'string', value: 'auto', description: 'The distro variant to use when rendering templates') | |||
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.
Since this is only required for building packages on different distros, it would be good to document why this option even exists when "auto" is the default.
|
Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close. If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging blackboxsw, and he will ensure that someone takes a look soon. (If the pull request is closed and you would like to continue working on it, please do tag blackboxsw to reopen it.) |
Restore the previous support for specifying the variant used in
template rendering instead of always depending on the host.
Before #5027 this was configured with the --distro option to setup.py,
so use the distro meson option.
Proposed Commit Message
Additional Context
Test Steps
Merge type