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

Allow to comment out or uncomment a macro definition #298

Merged
merged 1 commit into from
Nov 5, 2023

Conversation

nforro
Copy link
Member

@nforro nforro commented Nov 2, 2023

Necessary for Packit to be able to comment out or uncomment a pre-release suffix macro definition (example).

Related to packit/packit#2013.

RELEASE NOTES BEGIN

Macro definitions gained a new commented_out property indicating that a macro definition is commented out. Another new property, comment_out_style, determines if it is achieved by using a %dnl (discard next line) directive (e.g. %dnl %global prerelease beta2) or by replacing the starting % with # (e.g. #global prerelease beta2).

RELEASE NOTES END

Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/c5dad9d0aada4e89af1d6993a3c1326f

✔️ pre-commit SUCCESS in 1m 51s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 10s
✔️ specfile-tests-pip-deps SUCCESS in 58s

@gotmax23
Copy link
Contributor

gotmax23 commented Nov 2, 2023

Why not use the %dnl macro which doesn't require escaping anything to comment out lines? (Note that this isn't available on older RPM versions.)

@nforro
Copy link
Member Author

nforro commented Nov 2, 2023

Why not use the %dnl macro which doesn't require escaping anything to comment out lines? (Note that this isn't available on older RPM versions.)

Mainly because nobody uses %dnl to switch from a pre-release, whereas commenting out seems to be widely used in Fedora (see for example https://src.fedoraproject.org/rpms/cmake/c/4e11520f2d913bce471f53156dfe21e77bee8215). I don't know if this "convention" is documented somewhere, but a lot of packages seem to have adopted it and I don't think we can just change it.

@gotmax23
Copy link
Contributor

gotmax23 commented Nov 2, 2023

Well, if you're adding a commented_out property, %dnl %global ... and %dnl %define ... should also be considered.

@nforro
Copy link
Member Author

nforro commented Nov 2, 2023

Well, if you're adding a commented_out property, %dnl %global ... and %dnl %define ... should also be considered.

Ok, I suppose it would make sense to add another property, let's say disabled, for %dnl. Or would you suggest a better name?

@nforro nforro force-pushed the macro_definitions branch from fc4bf9c to 9c4d01d Compare November 2, 2023 17:49
@nforro nforro changed the title Allow to comment out or uncomment a macro definition Allow to disable or comment out a macro definition (and vice versa) Nov 2, 2023
Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/5a612de5c55e4f60b62b256ead4b13bc

✔️ pre-commit SUCCESS in 1m 54s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 04s
✔️ specfile-tests-pip-deps SUCCESS in 1m 03s

@gotmax23
Copy link
Contributor

gotmax23 commented Nov 2, 2023

Maybe have one commented_out attribute and another attribute that stores whether the comment character is # or %dnl? I don't personally see the value in differentiating the two comment styles with separate attributes, but if you think differently, I think disabled is a good name.

@nforro nforro force-pushed the macro_definitions branch from 9c4d01d to 21027a3 Compare November 3, 2023 09:24
@nforro nforro changed the title Allow to disable or comment out a macro definition (and vice versa) Allow to comment out or uncomment a macro definition Nov 3, 2023
Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/45bdffc31eb44dccad2e02e8a3cdf563

✔️ pre-commit SUCCESS in 2m 05s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 18s
✔️ specfile-tests-pip-deps SUCCESS in 1m 24s

Copy link
Member

@lachmanfrantisek lachmanfrantisek left a comment

Choose a reason for hiding this comment

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

I like the approach with multiple styles.

@nforro nforro added the mergeit Zuul, merge it! label Nov 5, 2023
Copy link
Contributor

Build succeeded (gate pipeline).
https://softwarefactory-project.io/zuul/t/packit-service/buildset/724ff1702a9446ffb14ca8a9130d1834

✔️ pre-commit SUCCESS in 1m 43s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 6abe398 into main Nov 5, 2023
31 checks passed
@delete-merged-branch delete-merged-branch bot deleted the macro_definitions branch November 5, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergeit Zuul, merge it!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants