eclipse-embed-cdt / eclipse-plugins

The Eclipse Embedded CDT plug-ins for Arm & RISC-V C/C++ developers (formerly known as the GNU MCU Eclipse plug-ins). Includes the archive of previous plug-ins versions, as Releases.
http://eclipse-embed-cdt.github.io/
Eclipse Public License 2.0
558 stars 130 forks source link

RISC-V toolchain definition bundle should not depend on UI #420

Closed ruspl-afed closed 3 years ago

ruspl-afed commented 3 years ago

Currently ilg.gnumcueclipse.managedbuild.cross.riscv has a number of UI dependencies that can and should be extracted to the separate bundle(s). The rationale behind this is to not enforce UI requirement for downstream solutions like https://github.com/openhwgroup/core-v-ide-cdt

Also headless implementation for this part can greatly simplify the configuration of unit-test pipeline and open the opportunity for CLI scenarios.

@ilg-ul do you think org.eclipse.embedcdt.managedbuild.cross.riscv.ui could be a good name for the bundle with UI part?

ilg-ul commented 3 years ago

The name seems ok, but extracting the UI part in a separate bundle seems to break compatibility with previous versions.

ruspl-afed commented 3 years ago

seems to break compatibility with previous versions.

Not necessary if we will be careful enough. I'm going to keep the identifiers and even package names where required.

jonahgraham commented 3 years ago

If the existing bundle becomes the UI bundle (no name change), then it can depend on a new core bundle, meaning that dependencies on the current bundle will work in upgrade scenarios without features.

ruspl-afed commented 3 years ago

So, you suggest to introduce org.eclipse.embedcdt.managedbuild.cross.riscv.core and try to move all the core contributions there (with proper reexport where needed), right?

jonahgraham commented 3 years ago

[...] right?

Yes.

ilg-ul commented 3 years ago

Let me summarise: in the current bundles we have two kind of classes, (core & ui), and other third party plug-ins import both kinds from such bundles.

If we move some of the classes in a separate bundle, they will no longer be at the expected place, and third party plug-ins will fail.

How do you plan to avoid this?

jonahgraham commented 3 years ago

I'll leave Alexander to put the concrete proposal in place (PR?) but it can be done with reexport directive in the Manifest.

See http://beyond-helloworld.blogspot.com/2014/08/using-reexport-with-require-bundle-in.html?m=1 see the section Another use of using visibility:reexport as that describes the use case here.

ilg-ul commented 3 years ago

If we are going to reorganise the code, we should do it for all plug-ins/features, and I'd prefer to have explicit names with core and ui.

The solution should not break compatibility for users, and it would be nice to also preserve compatibility for integrators, but only if the effort to do so is reasonable.

If we need to break compatibility for users, we have to plan it carefully, to be sure that all changes are in one release, to avoid having to do it again soon.

ruspl-afed commented 3 years ago

Well, doing it for all the plug-ins/features means that every file file be moved @ilg-ul Can we split this work (per feature?) to make the diff more manageable?

ilg-ul commented 3 years ago

Sure, but since this is a major change, I would like to have a clear plan for this.

I don't know how much time we have before the 2020-12 code freeze, so I'm a bit reluctant to do it now.

I was thinking to rename the features first, and have 2020-12 go out with the 'standard' Eclipse IDs, and reorganise the plug-ins in a second step.

I'm now testing 5.2.1, which was needed to accommodate some path changes in xPacks.

After this I planned to try to rename the features, and possibly release 5.2.2 to be included in 2020-12.

ruspl-afed commented 3 years ago

Does it mean that this issue should be postponed until we will start the repack of bundles?

ilg-ul commented 3 years ago

I don't know, I just want to do it right; 2020-12 is very close, it is the first release when the project becomes a real EF project, and I don't want to mess anything.

Do you have a better plan?

ruspl-afed commented 3 years ago

This is what I have in mind: 1) releng work: change maven identifiers to org.eclipse 2) features repackage: move to org.eclipse and add p2.inf 3) bundles repackage (symbolic names only): move to org.eclipse.*.ui and add p2.inf (we need to check if it works + check that plugin.xml contributions are not affected)

From this point I can proceed with extraction of "*.core" where needed.

And then the resource-intensive part:

4) java package renaming (check that plugin.xml contributions are not affected) 5) plugin.xml contributions repackaging: the most challenging, may need a lot of efforts to create converters

ilg-ul commented 3 years ago

releng work: change maven identifiers to org.eclipse

Hmmm... maven if far from my favourite tool, I don't know what identifiers you mean.

features repackage: move to org.eclipse and add p2.inf

I can try to add that update.matchExp and see if it helps, but I'm not sure I'm doing the right thing.

bundles repackage (symbolic names only): move to org.eclipse.*.ui and add p2.inf (we need to check if it works + check that plugin.xml contributions are not affected)

This exceeds my current experience with Eclipse.

Also 4 & 5 will be painful.

From my point of view, it would be great to have 2 addressed before 2020-12, and I can try to experiment with update.matchExp. Hopefully Marc will help if needed.

For all the other points, we'll address them when you and Jonah will consider reasonable, my only concern is rushing before a significative release and messing things.

ilg-ul commented 3 years ago

I created [#421] to keep track to the features rename details.

ilg-ul commented 3 years ago

I have difficulties to identify the classes that should go to the .riscv.core plug-in.

Renaming the current one as org.eclipse.embedcdt.managedbuild.cross.riscv.ui is easier now, but most of the code seems related in one way or another to the UI, and I have no idea what you would expect to be part of a headless operation, or to a derived plug-in.

Please suggest how to proceed.

jonahgraham commented 3 years ago

@ruspl-afed can hopefully chime in on what part of Embed-CDT is not working in his use case outlined in the OP.

I had a look at the plug-in, and while there are indeed UI elements in the plugin.xml and other places, as long as it depends on org.eclipse.cdt.managedbuilder.core then there will be a transitive UI dependency as org.eclipse.cdt.managedbuilder.core depends on org.eclipse.ui. Same thing goes for org.eclipse.cdt.managedbuilder.gnu.ui - the org.eclipse.cdt.managedbuilder.core.buildDefinitions that I assume RISCV are dependent on come from the .gnu.ui CDT bundle.

The org.eclipse.cdt.managedbuilder bundles are almost UI free, but a number of changes would be needed to make them fully UI free.

@ruspl-afed I was wondering which specific parts you want pulled into the core bundle? And further are you looking at make a runtime that does not have any dependency on org.eclipse.swt at all? i.e. I am trying to understand the difference between the ideal design (proper core/ui split) and the practical problem you are facing. I couldn't tell from https://github.com/openhwgroup/core-v-ide-cdt what they may be.

ruspl-afed commented 3 years ago

@ilg-ul initially I planned to provide a PR for this issue, but it triggered too much related things.

I have difficulties to identify the classes that should go to the .riscv.core plug-in.

Basically everything that is free from

The problem is Activator extends AbstractUIActivator - it should be split for core and UI as well.

there will be a transitive UI dependency as org.eclipse.cdt.managedbuilder.core depends on org.eclipse.ui.

You are right @jonahgraham , but these UI usages are not in the API and can be eliminated without CDT API break at any moment.

are you looking at make a runtime that does not have any dependency on org.eclipse.swt at all

yes, this is the practical part, I would like to avoid failures for UI-less environments.

ilg-ul commented 3 years ago

I'll take a second look, but the point is that there are other classes that are free of that list, but functionally make no sense alone, they are tightly coupled to UI content, for example the classes that manage configurations.

Today I'll try to finalise everything else and then allow you to work on a PR.

ruspl-afed commented 3 years ago

@ilg-ul I prepared an MR to agree on how to arrange the changes. Perhaps you would like to rename the headless part to "*.core" and to have a dedicated branding bundle, i.e.:

ilg-ul commented 3 years ago

riscv, riscv.core, riscv.ui

Yes, this is a good idea.

If you update it to use the develop-split branch, i'll merge it and take a look.

We have to do the same for cross.arm, and the other plug-ins.

ilg-ul commented 3 years ago

I guess #441 & the following fixed this.