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

b4.4 bug: deprecate -t since it is gone in main, mixes poorly with -l, and exits when there is no preexisting log #1488

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

baallan
Copy link
Collaborator

@baallan baallan commented Oct 29, 2024

truncating the log now works correctly whether the old log exists or not and independent of the ordering of -l and -t commands.

@baallan
Copy link
Collaborator Author

baallan commented Oct 29, 2024

fixes #1443

@baallan baallan requested a review from nichamon October 29, 2024 21:24
@baallan baallan added this to the v4.4.5 milestone Oct 29, 2024
@baallan baallan changed the title fix -t option to work without dependence on order or log file preexistence b4.4 bug: fix -t option to work without dependence on order or log file preexistence Oct 29, 2024
Copy link
Collaborator

@nichamon nichamon left a comment

Choose a reason for hiding this comment

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

While the change fixes the order dependency of -t and -l when they are specified at the command line, -t won't be supported when it is given in configuration files.

ldms/src/ldmsd/ldmsd.c Outdated Show resolved Hide resolved
@tom95858
Copy link
Collaborator

tom95858 commented Nov 1, 2024

@baallan, @nichamon this 'feature' has been removed from top of tree. Should we consider getting rid of this in 4.4? I know it's convenient to crush the log file at start up, but it can trivially be achieved in other ways with the new 'rm' command ;-)

@baallan
Copy link
Collaborator Author

baallan commented Nov 1, 2024

@tom95858 If it is gone from main, I'm fine with making it a no-op+warning in 4.4. Just deleting it could break working scripts. But in theory this should be a community decision?

A patch to do that might also add deprecation warnings in 4.4 about all other options that are going away/gone in main.

@baallan
Copy link
Collaborator Author

baallan commented Nov 4, 2024

@tom95858 revised this pr to deprecate -t.

@baallan baallan changed the title b4.4 bug: fix -t option to work without dependence on order or log file preexistence b4.4 bug: deprecate -t since it is gone in main, mixes poorly with -l, and exits when there is no preexisting log Nov 4, 2024
@nichamon
Copy link
Collaborator

nichamon commented Nov 5, 2024

@baallan Thanks for making the revisions. Given that your latest commit removes the -t option completely, would you consider dropping the first commit dcc50b5?

@baallan
Copy link
Collaborator Author

baallan commented Nov 5, 2024

@nichamon if we could just check the squash box on the pr, that should take care of it. I don't have any pull/merge controls on the github repo, however.

This turns -t into a no-op and a warning.
@baallan
Copy link
Collaborator Author

baallan commented Nov 5, 2024

@nichamon found the branch and squashed it.

@tom95858 tom95858 merged commit 97c87ac into ovis-hpc:b4.4 Nov 12, 2024
14 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