8000 fix(pom): remove classpath manifest entry by mwangggg · Pull Request #311 · cryostatio/cryostat-agent · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(pom): remove classpath manifest entry #311

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 7, 2024
Merged

fix(pom): remove classpath manifest entry #311

merged 1 commit into from
Feb 7, 2024

Conversation

mwangggg
Copy link
Member

fixes: #279
I also replaced the deprecated OperatingSystemMXBean methods.

@mwangggg mwangggg requested a review from a team as a code owner December 24, 2023 03:53
@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Dec 24, 2023
@andrewazores
Copy link
Member

I also replaced the deprecated OperatingSystemMXBean methods.

Could you split these changes into a separate PR? I think we will want the classpath manifest piece to go into the next bugfix release, but the MXBean changes would need to wait for the next major or minor release.

@andrewazores andrewazores removed the needs-triage Needs thorough attention from code reviewers label Jan 2, 2024
@andrewazores
Copy link
Member

Actually, it looks like the MXBean changes would need to wait until we drop JDK 11 support:

 Error:  COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
Error:  /home/runner/work/cryostat-agent/cryostat-agent/src/main/java/io/cryostat/agent/model/MBeanInfo.java:[81,57] error: invalid method reference
  cannot find symbol
    symbol:   method getFreeMemorySize()
    location: interface UnixOperatingSystemMXBean
Error:  /home/runner/work/cryostat-agent/cryostat-agent/src/main/java/io/cryostat/agent/model/MBeanInfo.java:[85,48] error: invalid method reference
  cannot find symbol
    symbol:   method getCpuLoad()
    location: interface UnixOperatingSystemMXBean
Error:  /home/runner/work/cryostat-agent/cryostat-agent/src/main/java/io/cryostat/agent/model/MBeanInfo.java:[86,58] error: invalid method reference
  cannot find symbol
    symbol:   method getTotalMemorySize()
    location: interface UnixOperatingSystemMXBean

@mwangggg mwangggg force-pushed the 279-classpath-fix branch 2 times, most recently from 5c9ad08 to 53f181d Compare January 15, 2024 15:55
andrewazores
andrewazores reviewed Jan 15, 2024
View reviewed changes
@mwangggg mwangggg force-pushed the 279-classpath-fix branch 3 times, most recently from 79e6ef7 to 0acd2c6 Compare January 16, 2024 16:38
@andrewazores
Copy link
Member

Looks good, at least when using the -shaded version of the Agent JAR. I haven't yet tested it with the non-shaded version but I suspect we might need to make this manifest attribute conditionally included only for that non-shaded form.

@andrewazores
Copy link
Member

Maybe we need to use a -Pshaded Maven profile for building shaded vs non-shaded. That way we can move this configuration into one configuration or the other so that the manifest gets built according to which way the JAR was built. @ebaron WDYT? Right now the build doesn't use any profiles, it just produces the plain, -sources, and -shaded JARs every time.

@ebaron
Copy link
Member
ebaron commented Jan 23, 2024

If there's some value in retaining the Class-Path attribute for non-shaded jars, then a profile could work. Although, even when building a shaded jar, the non-shaded jar is built too. We'd probably need to have it do both.

@andrewazores
Copy link
Member

I tried to put together a quick test of a sample application that consumes the non-shaded JAR, and it ended up with the application correctly loading the agent JAR but not having any of the agent dependencies on the classpath, so the agent failed miserably. That could be worked around at the application level by adding all of the agent's dependencies to the application classpath as well, but I think/hope that retaining the classpath manifest entry will avoid that step.

Then again, our primary usecase is for the shaded JAR, and I'm not sure how big of a hurdle it is for any users who do need the non-shaded JAR to also have to figure out the agent dependency management, too. If this ends up being too much work then maybe we just go without it for now and wait until someone actually asks for it.

@andrewazores andrewazores merged commit ea171de into cryostatio:main Feb 7, 2024
mergify bot pushed a commit that referenced this pull request Feb 7, 2024
@mwangggg @mergify
fix(pom): remove classpath manifest entry (#311)
5ca543c 
(cherry picked from commit ea171de)
@mergify mergify bot mentioned this pull request Feb 7, 2024
Merged
andrewazores pushed a commit that referenced this pull request Feb 7, 2024
@mergify @mwangggg
fix(pom): remove classpath manifest entry (#311) (#332)
1906c19 
(cherry picked from commit ea171de)

Co-authored-by: Ming Yu Wang <90855268+mwangggg@users.noreply.github.com>
@ebaron ebaron mentioned this pull request Feb 26, 2024
Merged
@ebaron ebaron mentioned this pull request Apr 5, 2024
Open
@mwangggg mwangggg deleted the 279-classpath-fix branch June 27, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewazores andrewazores andrewazores approved these changes

@aali309 aali309 Awaiting requested review from aali309

Assignees
No one assigned
Labels
backport fix safe-to-test
Projects
No open projects
2.4.1 release
Status: Done
3.0.0 release
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Shaded JAR Manifest includes Class-Path entry
3 participants
@mwangggg @andrewazores @ebaron
0