jiekang commented 4 years ago

The project currently uses classes/packages from jmc that aren't part of jmc-core, the api bundle for jmc. These should be requested in jmc upstream to be moved into jmc-core (if appropriate).

This is a decent time to make the request as JMC is now on 8.0.0-SNAPSHOT, which suggests api changes like this could be made.

jiekang commented 4 years ago

@andrewazores Do you happen to have a list handy of the exact fully qualified class names this project uses that are in jmc, not jmc-core?

andrewazores commented 4 years ago

I don't. The only list of classes I had [0] looked at were the ones in container-jfr which were from JMC, but not whether those came from jmc or jmc-core.

[0] https://github.com/rh-jmc-team/container-jfr/issues/59#issuecomment-551159163

andrewazores commented 4 years ago

And that's a list of the classes that at that time were still not abstracted over and moved into this -core project, so the full set is larger than that since it needs to be unioned with the set of classes used in this codebase. The same or similar ag/grep/etc. is handy enough for getting that class list.

jiekang commented 4 years ago

Ah so I should actually look at both container-jfr and container-jfr-core. Okay thanks!

andrewazores commented 4 years ago

Yea, not everything has been moved into -core yet. Ideally all of the jmc API access is abstracted over or reimplemented by -core and the main container-jfr codebase contains zero references to any JMC classes, but that isn't the case right now.

jiekang commented 4 years ago

List of imports not in jmc-core:

import org.openjdk.jmc.flightrecorder.configuration.events.EventOptionID;
import org.openjdk.jmc.flightrecorder.configuration.events.IEventTypeID;
import org.openjdk.jmc.flightrecorder.configuration.recording.RecordingOptionsBuilder;

import org.openjdk.jmc.jdp.client.JDPClient;

import org.openjdk.jmc.rjmx.ConnectionDescriptorBuilder;
import org.openjdk.jmc.rjmx.ConnectionToolkit;
import org.openjdk.jmc.rjmx.IConnectionDescriptor;
import org.openjdk.jmc.rjmx.IConnectionHandle;
import org.openjdk.jmc.rjmx.IConnectionListener;
import org.openjdk.jmc.rjmx.internal.DefaultConnectionHandle;
import org.openjdk.jmc.rjmx.internal.RJMXConnection;
import org.openjdk.jmc.rjmx.internal.ServerDescriptor;
import org.openjdk.jmc.rjmx.internal.WrappedConnectionException;
import org.openjdk.jmc.rjmx.services.internal.CommercialFeaturesServiceFactory;

import org.openjdk.jmc.rjmx.services.jfr.FlightRecorderException;
import org.openjdk.jmc.rjmx.services.jfr.IEventTypeInfo;
import org.openjdk.jmc.rjmx.services.jfr.IFlightRecorderService;
import org.openjdk.jmc.rjmx.services.jfr.internal.FlightRecorderServiceFactory;
import org.openjdk.jmc.rjmx.services.jfr.internal.FlightRecorderServiceV2;
import org.openjdk.jmc.rjmx.services.jfr.IRecordingDescriptor;
import org.openjdk.jmc.rjmx.services.jfr.IRecordingDescriptor.RecordingState;

import org.openjdk.jmc.ui.common.security.ActionNotGrantedException;
import org.openjdk.jmc.ui.common.security.FailedToSaveException;
import org.openjdk.jmc.ui.common.security.ISecurityManager;
import org.openjdk.jmc.ui.common.security.SecurityException;

jiekang commented 4 years ago

List of imports in jmc-core:

import org.openjdk.jmc.common.unit.IConstrainedMap;
import org.openjdk.jmc.common.unit.IConstraint;
import org.openjdk.jmc.common.unit.IDescribedMap;
import org.openjdk.jmc.common.unit.IMutableConstrainedMap;
import org.openjdk.jmc.common.unit.IOptionDescriptor;
import org.openjdk.jmc.common.unit.IQuantity;
import org.openjdk.jmc.common.unit.QuantityConversionException;
import org.openjdk.jmc.common.unit.UnitLookup;

import org.openjdk.jmc.flightrecorder.CouldNotLoadRecordingException;
import org.openjdk.jmc.flightrecorder.JfrLoaderToolkit;

ebaron commented 3 years ago

56 has a temporary fix for this, by maintaining an in-tree copy of JMC application classes that we use. A more permanent fix would be refactoring JMC upstream to move this functionality to core.

aptmac commented 7 months ago

Last week the PRs for JMC-7308 (flightrecorder.configuration) [0] and JMC-7069 (rjmx.common) [1] were merged, which moves the classes listed in the comments above into the JMC core libraries.

These changes will be a part of the JMC 9 release, and should be available through maven central in the new year. JMC 9 is currently set to have a source release on January 24, 2024 [2].

I'll revisit these once the maven central release happens, but here are some branches that I prepared to make sure the cryostat repos build and pass all their unit + itests.

(2024-01-23: these branches are up-to-date and use the Adoptium JMC core snapshot jars) cryostat-core: https://github.com/aptmac/cryostat-core/tree/core-9.0.0-SNAPSHOT cryostat-reports: https://github.com/aptmac/cryostat-reports/tree/core-9.0.0-SNAPSHOT cryostat: https://github.com/aptmac/cryostat/tree/core-9.0.0-SNAPSHOT cryostat3: https://github.com/aptmac/cryostat3/tree/core-9.0.0-SNAPSHOT

These branches can also go towards: https://github.com/cryostatio/cryostat-core/issues/173

[0] https://bugs.openjdk.org/browse/JMC-7308 [1] https://bugs.openjdk.org/browse/JMC-7069 [2] https://wiki.openjdk.org/display/jmc/Releases

aptmac commented 2 days ago

Just noticed that there were a couple of issues still open related to the dependency on jmc/application.

This was addressed in commit https://github.com/cryostatio/cryostat-core/commit/34c5e0e423069c748553a4a51c4c3f5c9fda5e54.

jiekang commented 2 days ago

Yay!

cryostatio / cryostat-core

Project relies on jmc classes that aren't part of jmc-core #22

56 has a temporary fix for this, by maintaining an in-tree copy of JMC application classes that we use. A more permanent fix would be refactoring JMC upstream to move this functionality to core.