Skip to content

Remove jvmrunargs lookup #3874

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

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

ramanathan1504
Copy link

@ramanathan1504 ramanathan1504 commented Aug 8, 2025

jvmrunargs was added in 2014-08-20 (1f64e68), and never worked until it got accidentally fixed in 2.25.0 (see #3071 and f7c26cd), which was released in 2025-06-13. We decided to remove it1.

1 Link to the internal Slack conversation. (Only accessible by certain ASF committers and some limited set of guests.)

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

Hi @ramanathan1504,

Thanks for the PR! 💯

From what I can tell, the jvmrunargs lookup was already fixed in version 2.25.0, so the main code changes here may not be needed. However, the unit tests you’ve added are very valuable. They’ll help ensure the lookup keeps working. The absence of such tests is likely why a feature that never fully worked managed to ship in so many releases.

Could you:

  • Remove the main code changes but keep JvmRunArgsLookupTest.
  • Consider adding documentation for jvmrunargs (either in this PR or in a follow-up). See #3876 for details.

Copy link

github-actions bot commented Aug 10, 2025

Job Requested goals Build Tool Version Build Outcome Build Scan®
build-macos-latest clean install 3.9.8 Build Scan PUBLISHED
build-ubuntu-latest clean install 3.9.8 Build Scan PUBLISHED
build-windows-latest clean install 3.9.8 Build Scan PUBLISHED
Generated by gradle/develocity-actions

Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

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

@ramanathan1504, would you mind implementing the following changes, please?

  1. Remove log4j2-test.xml that you added
  2. Remove o.a.l.l.core.lookup.Interpolator::LOOKUP_KEY_JVMRUNARGS field
  3. Remove o.a.l.l.core.lookup.JmxRuntimeInputArgumentsLookup
  4. Move the changelog entry file to src/changelog/.2.x.x/3874_remove_jvmrunargs_lookup.xml
  5. Fix the changelog entry file syntax (see 3176_validate_scripts_in_ScriptsPlugin.xml for an example)
  6. Set the entry.type to removed in the changelog entry file

@vy vy changed the title Fix: Ensure jvmrunargs lookup is always registered (fixes #2726) Remove jvmrunargs lookup Aug 14, 2025
Copy link
Author

@ramanathan1504 ramanathan1504 left a comment

Choose a reason for hiding this comment

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

Fixes #3874

  • Removed log4j2-test.xml
  • Removed LOOKUP_KEY_JVMRUNARGS field from Interpolator
  • Removed JmxRuntimeInputArgumentsLookup class
  • Moved and updated the changelog entry to src/changelog/.2.x.x/3874_remove_jvmrunargs_lookup.xml with the correct format and type

Please review and let me know if any further changes are needed.

Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

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

@ramanathan1504, could you remove JmxRuntimeInputArgumentsLookup, please? (Please review the Files changed tab in GitHub after pushing your changes.)

@ramanathan1504
Copy link
Author

Understood, I’ll remove it. I’ll be more careful in future PRs to ensure such issues don’t occur again.

ramanathan1504 added a commit to ramanathan1504/logging-log4j2 that referenced this pull request Aug 15, 2025
- Removed JmxRuntimeInputArgumentsLookup class
- Updated the changelog entry to src/changelog/.2.x.x/3874_remove_jvmrunargs_lookup.xml
@vy
Copy link
Member

vy commented Aug 15, 2025

@ramanathan1504, you need to sign your commits. The "In a GitHub PR, how can I sign (not sign-off!) existing commits?" prompt creates a good enough ChatGPT response.

Note that since you'll be truncating the history anyway, you can as well squash all your changes into a single commit, sign it, and force-push it.

I’ll be more careful in future PRs to ensure such issues don’t occur again.

No worries. Shit happens.

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

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

Hi @ramanathan1504,

Thanks for the contribution!

Removing a class is technically a breaking change, which is why the BND Baseline check fails.

However, since we’ve decided this particular removal should not be treated as a breaking change, you’ll need to override the check by doing the following:

  1. Identify the element that contains the removed class. For classes, this means the containing package:

    @Export
    @Version("2.24.1")
    package org.apache.logging.log4j.core.lookup;
    import org.osgi.annotation.bundle.Export;
    import org.osgi.annotation.versioning.Version;

  2. Add a @BaselineIgnore("2.26.0") annotation to that package. This tells BND to ignore the semver violation up to version 2.26.0 of log4j-core.

  3. Even though we’re not treating this as a MAJOR change, it is a MINOR change for the package. Per OSGi semantic versioning, you must bump the package version (2.24.1) by at least one minor version.
    Our convention is to align the package version with the library version, so update it to @Version("2.26.0"). This way, the package version clearly answers the question: “In which Log4j version did the last API change in this package occur?”

After these annotation changes, please run the build to identify and remove the tests that rely on the removed lookup.

@ramanathan1504
Copy link
Author

Hi @ppkarwasz,

Thanks for the detailed instructions! I’ll make the changes as suggested:
• Add @BaselineIgnore("2.26.0") to the package.
• Bump the package version to @Version("2.26.0").
• Update/remove tests that rely on the removed class.

I’ll push the updates shortly.

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

Successfully merging this pull request may close these issues.

3 participants