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

LOGROTATE: fix path to pid file #7787

Closed

Conversation

alexey-tikhonov
Copy link
Member

No description provided.

@alexey-tikhonov alexey-tikhonov marked this pull request as ready for review January 7, 2025 13:29
@@ -5253,6 +5253,7 @@ edit_cmd = $(SED) \
-e 's|@localstatedir[@]|$(localstatedir)|g' \
-e 's|@runstatedir[@]|$(runstatedir)|g' \
-e 's|@logpath[@]|$(logpath)|g' \
-e 's|@pidpath[@]|$(pidpath)|g' \
Copy link
Contributor

Choose a reason for hiding this comment

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

This question has nothing to do with your PR. I'm just trying to understand why it is done this way everywhere.

Why is the second @ between square brackets? Considering these are regular expressions, the character in that position must be present in the set between the brackets. In his case the set is composed of a single @, which make it equivalent to using the @ without the brackets (as was done for the first @). So why using the brackets?

Copy link
Member Author

Choose a reason for hiding this comment

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

It just doesn't work without '[]' (variable isn't substituted). Not sure why though.

Copy link
Contributor

@aplopez aplopez Jan 8, 2025

Choose a reason for hiding this comment

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

Surprising. When I try it on the command line, it works without the brackets too.

Copy link
Contributor

Choose a reason for hiding this comment

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

The string is actually expanded several times before running.

First the Makefile expansion is done. Autotools also can expand @variable@ in makefile and configure. So I guess this the issue we are facing/workarounding here.

Copy link
Contributor

@aplopez aplopez left a comment

Choose a reason for hiding this comment

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

ACK

Copy link
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

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

Thanks, ACK

@alexey-tikhonov alexey-tikhonov added the Ready to push Ready to push label Jan 8, 2025
@alexey-tikhonov
Copy link
Member Author

Pushed PR: #7787

  • master
    • 6bd231c - LOGROTATE: fix path to pid file
  • sssd-2-10
    • 12bd585 - LOGROTATE: fix path to pid file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants