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 RPM package release and version files expansion #816

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

k0walik
Copy link
Contributor

@k0walik k0walik commented Feb 5, 2024

What

While trying to migrate from version 0.9.1 to 0.10.0 I stumbled across following error:

Traceback (most recent call last):
        File "/home/tomasz.wojno/code/rules_pkg/pkg/rpm_pfg.bzl", line 352, column 47, in _pkg_rpm_impl
                content = substitute_package_variables(ctx, "\n".join(preamble_pieces)),
        File "/home/tomasz.wojno/code/rules_pkg/pkg/private/util.bzl", line 95, column 34, in substitute_package_variables
                return attribute_value.format(**vars)
Error in format: Missing argument 'VERSION_FROM_FILE'

In #787 a helper function for expanding RPM preamble with user-defined variables was introduced. The function uses Starlark format expression to replace {var} occurrences with defined variables. However, this does not work when we pass release or version file to the build target. The rpm_pfg.bzl appends Release: ${RELEASE_FROM_FILE} and Version: ${VERSION_FROM_FILE} to preamble, which are later expanded by make_rpm.py script. Thus, those variables are failed to be expanded as they are not defined.

Changes

This pull request introduces following changes:

  • Avoid expanding RELEASE_FROM_FILE and VERSION_FROM_FILE by wrapping them in double curly braces. This way ${{RELEASE_FROM_FILE}} becomes expanded to ${RELEASE_FROM_FILE}, so it can be processed later by make_rpm.py
  • Add tests that uses version_file and release_file attributes
  • Fix passing changelog file to OutputGroupInfo

Copy link
Collaborator

@cgrindel cgrindel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@cgrindel
Copy link
Collaborator

cgrindel commented Feb 6, 2024

@aiuto Do you want to review or can I merge this PR?

Copy link
Collaborator

@aiuto aiuto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding good tests along with this.

@aiuto aiuto merged commit 37cccd4 into bazelbuild:main Feb 8, 2024
2 checks passed
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.

3 participants