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

Computation of Exclude/Include patterns in a non-persitent way by an extension #642

Open
laeubi opened this issue Jan 16, 2023 · 19 comments

Comments

@laeubi
Copy link
Contributor

laeubi commented Jan 16, 2023

Currently I can define exclude/include items from classpath by specifying a pattern:

grafik

this is then stored in the .classpath like this:

<classpathentry excluding="**/example/*.java" kind="src" output="target/test-classes" path="src/test/java">

What I like to archive is, that this value is not persistently stored in the classpath, but computed by an extension point, such an extension point should get the IClasspath element (and if possible the I(Java)Project) and would return then include/exclude patterns that should apply to this classpath element.

@stephan-herrmann what do you think? This would be a more generic replacement of the enable/disable, and actually we already use this in m2e, but only compute it once and store it in the .classpath, this would also help with:

PDE can simply then implement this extension and filter out all classes that do not match the current target environment.

One problem I see is that when to actually this must be applied, e.g. when resolving classpath? but this is also shown in the UI as "disabled", so the UI must probably also call the extension point.

@iloveeclipse
Copy link
Member

Isn't there already IClasspathContainer for that?

@laeubi
Copy link
Contributor Author

laeubi commented Jan 17, 2023

Isn't there already IClasspathContainer for that?

IClasspathcontainer hosts the classpath entries yes, but I'm not sure how it helps here?

@stephan-herrmann
Copy link
Contributor

Isn't there already IClasspathContainer for that?

IClasspathcontainer hosts the classpath entries yes, but I'm not sure how it helps here?

The mechanism of classpath container and classpath container initializer translates ("resolves") one container entry into a set of concrete entries, typically jars or other projects. The cool thing here is, that the extension implementing container resolution is free to create the concrete classpath entries with any details they wish, e.g, the PDEClasspathContainer interprets MANIFEST.MF not only for identifying all required bundles, but also adds access rules to each entry, reflecting the details of the target manifest (x-internal and x-friends).

For a recent example you may have a look at Bug 526011 (gerrit) where I use a new manifest entry to generate an extra classpath atttribute "annotationPath".

In short, any information available to the extension (implementation of a classpath container) and which can be represented already in terms of classpath attributes, can be dynamically passed into JDT with this mechanism. Plus: JDT even remembers attributes on resolved classpath entries in its persistent State, where resolution of classpath containers is cached for improved start-up time.

Thanks, @iloveeclipse for reminding us of this powerful concept :)

@laeubi
Copy link
Contributor Author

laeubi commented Jan 17, 2023

Well but I don't want to implement my own ClasspathContainer I want to modify the behavior of JDT ones (see screenshot above) or probably all (if include/exclude applies to every classpath container).

@stephan-herrmann
Copy link
Contributor

(see screenshot above)

The entry is partly hidden, but you seem to be talking about a source entry, not a container entry, right?

I wonder if it would be possible to wrap even source folders inside a new filtering classpath container. Let me see ...

@laeubi
Copy link
Contributor Author

laeubi commented Jan 17, 2023

(see screenshot above)

The entry is partly hidden, but you seem to be talking about a source entry, not a container entry, right?

I wonder if it would be possible to wrap even source folders inside a new filtering classpath container. Let me see ...

IClasspathEntry can be of type

  • CPE_LIBRARY
  • CPE_PROJECT
  • CPE_SOURCE
  • CPE_VARIABLE
  • CPE_CONTAINER

and has the getExcludePattern() / getIncludePattern()

IClasspathContainer.getClasspathEntries() as per javadoc only contains CPE_LIBRARY or CPE_PROJECT so I'm not sure if that helps me here.

@stephan-herrmann
Copy link
Contributor

IClasspathContainer.getClasspathEntries() as per javadoc only contains CPE_LIBRARY or CPE_PROJECT so I'm not sure if that helps me here.

I could not find any code enforcing this. So perhaps this is just how it was used up until now?

@stephan-herrmann
Copy link
Contributor

I could not find any code enforcing this. So perhaps this is just how it was used up until now?

Correction: org.eclipse.jdt.internal.core.ClasspathEntry.validateClasspathEntry(IJavaProject, IClasspathEntry, IClasspathContainer, boolean, boolean) would need to be modified. But I think this could be possible without negative impact.

@stephan-herrmann
Copy link
Contributor

Looking at this validation

	if (containerEntry == null
		|| kind == IClasspathEntry.CPE_SOURCE
		|| kind == IClasspathEntry.CPE_VARIABLE
		|| kind == IClasspathEntry.CPE_CONTAINER){
			return new JavaModelStatus(IJavaModelStatusConstants.INVALID_CP_CONTAINER_ENTRY, project, path);
	}

the purpose of disallowing CPE_VARIABLE and CPE_CONTAINER seems to be to avoid the need of nested resolving (i.e., entries of a resolved container should be directly usable).

Excluding also CPE_SOURCE might just have been a conservative approach, since no use case existed for this?

Should we try to remove just that line and play with a container wrapping source folders?

@stephan-herrmann
Copy link
Contributor

I found one location assuming that CPE_SOURCE can be identified using the raw (unresolved) classpath: org.eclipse.jdt.internal.core.JavaProject.isOnClasspath(IJavaElement), the following snippet would need an update, too:

		// no need to go further for compilation units and elements inside a compilation unit
		// it can only be in a source folder, thus on the raw classpath
		if (isSource)
			return false;

but that looks feasible, if you ask me.

@laeubi
Copy link
Contributor Author

laeubi commented Jan 17, 2023

I'm not sure, as I said I don't want a custom container, I just want to specific (dynamically) include/excludes for the existing JDT (source?) classpath entry :-)

@stephan-herrmann
Copy link
Contributor

I'm not sure, as I said I don't want a custom container, I just want to specific (dynamically) include/excludes for the existing JDT (source?) classpath entry :-)

You make it sound like inventing a new extension point is easier than using (and marginally expanding) an existing one. I believe the opposite is the case. :)

@laeubi
Copy link
Contributor Author

laeubi commented Jan 17, 2023

Bu what extension point do you like to expand? Sources are not Classpathcontainers, and even if they would it won't help me as I don't want to add one :-)

Look for example at org.eclipse.jdt.internal.corext.buildpath.ClasspathModifier.isExcluded(IResource, IJavaProject) ther it checks if an item is excluded (and there is isIncluded as well) and thats effectivly where I need to intercept, so:

Lets say isExcluded currently return true at the momment, I'd like my extension to be asked so I can change the answer to false ...

@stephan-herrmann
Copy link
Contributor

Bu what extension point do you like to expand?

It's not about like or not, but about assessing different possible solutions for a given requirement. For the concrete purpose here I suggested to consider lifting just one restriction of IClasspathContainer.getClasspathEntries() such that it admits CPE_SOURCE, too. That would increase the flexibility of the existing API to cover your use case with no new API, at least no new extension point.

it won't help me as I don't want to add one :-)

Well, if you already know which solution you don't want, then finding the most appropriate solution may be difficult.

My reasoning is based on the idea that adding / modifying API / extension points is much, much more harder to get right, than adding implementation against existing API. And if we release API that is not good, then our options to improve are not great.

As we have been discussing related topics in different issues now, I would suggest, to list all current and known future requirements in one place, and collectively try to find the smallest API surface to cover them all. Will you join me in this?

@laeubi
Copy link
Contributor Author

laeubi commented Jan 17, 2023

For the concrete purpose here I suggested to consider lifting just one restriction of IClasspathContainer.getClasspathEntries() such that it admits CPE_SOURCE, too.

But just assume IClasspathContainer admits CPE_SOURCE how will it help with the case having additional filtering for what is currently there? This is not related about the container at all... the container can stay as is.

As we have been discussing related topics in different issues now, I would suggest, to list all current and known future requirements in one place, and collectively try to find the smallest API surface to cover them all. Will you join me in this?

Just let me know where to join in ;-)

@iloveeclipse
Copy link
Member

The idea is that similar to PDE's "org.eclipse.pde.core.requiredPlugins" the maven provider dynamically computes everything, but now also where sources are.
So maven / m2e does all the classpath work instead of JDT specifying that in the .classpath.

@laeubi
Copy link
Contributor Author

laeubi commented Jan 17, 2023

The idea is that similar to PDE's "org.eclipse.pde.core.requiredPlugins" the maven provider dynamically computes everything, but now also where sources are.

m2e already has a classpath container (named "Maven Dependencies"), so you mean one should remove the items in .classpath and instead return CPE_SOURCE entries (together with the jar items)? If yes I think that could be interesting, and if you think JDT can handle this ... even PDE has "special" files (build.properties) already that duplicates some information from the classpath.

I'm just was curious because classpath kind="src" looks so general and special so I can't really tell if that could cause trouble somewhere but trust you and @stephan-herrmann as the JDT experts here 👍

@stephan-herrmann
Copy link
Contributor

stephan-herrmann commented Oct 26, 2024

@laeubi, is this issue still relevant for you? Don't you see a connection to our discussion at OCX 2024?

In particular eclipse-tycho/tycho#632 looks related to building a multi-release project / jar, no? Within one projects several parts should be compiled in separated builds using separate configurations.

Would osgi-environments qualify as classifiers to be attached to source folders?

Or are simple classifiers insufficient and we should follow up from isVisible() a la eclipse-tycho/tycho#632 (comment) (#614 ?)

OTOH, if the benefit is only to avoid copying .classpath files, and the effort is significant, please just close this request. Thanks.

@laeubi
Copy link
Contributor Author

laeubi commented Oct 27, 2024

Yes I think this is still interesting, e.g. maven has the concept of profiles where also (depending on system properties, OS version, jvm, ...) certain things needs to be enabled/disabled. Having "dynamic source entries" as proposed might be an option but I was not able to further instigate this yet.

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

No branches or pull requests

3 participants