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
555 stars 130 forks source link

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

Closed ruspl-afed closed 3 years ago

ruspl-afed commented 3 years ago

Extracted UI part to "org.eclipse.embedcdt.managedbuild.cross.riscv.ui"

Signed-off-by: Alexander Fedorov alexander.fedorov@arsysop.ru

ilg-ul commented 3 years ago

Hi Alexander, thank you for the contribution.

I created a new branch develop-split to have a place where we can test these changes, could you update the PR to use it instead of develop, which is (hopefully) stable now?

ilg-ul commented 3 years ago

I did a manual merge locally and took a look, there is not much left in the non-ui plug-in.

And I think the idea to have .riscv as a separate branding plugin and .riscv.core with the non-ui classes is valuable, and we should use it for all plug-ins.

Could you update the RISC-V plug-in with these changes, so I have a model to update the Arm one too?

ilg-ul commented 3 years ago

there is not much left in the non-ui plug-in.

Actually there are a lot of classes in the non-ui plug-in, I did not check carefully first. :-(

Can you remind me the procedure you used to discriminate between classes? Was it based only on dependencies on org.eclipse.ui, org.eclipse.ui, org.eclipse.jface, org.eclipse.swt, org.eclipse.cdt.ui, etc, without any relationship to the functionality?

Since in many cases the functionality of the core classes is closely related to the ui classes, and the assumption that the core classes can be used with another plug-in, implementing a different UI, is a gamble which might not pay.

Actually, except structure/aesthetics, what would be the practical advantages of the split based on this criteria?

ruspl-afed commented 3 years ago

Yes, the UI may be different if needed, but I don't think that it should change the semantic of toolchain definition that is done in "core" part. Also, I didn't found the bi-directional relation between UI and core classes.

The practical advantage is to enable usage of core part (toolchain definition and related code) in headless environment, the very first scenario is to enable unit testing for pipeline.

ilg-ul commented 3 years ago

Could you also rename to .riscv.core and extract the branding plug-in as .riscv?

ilg-ul commented 3 years ago

The initial preferences were linked to the unique plug-in and thus were prefixed with ilg.gnumcueclipse.managedbuild.cross.riscv.

Now, that we have 3 separate plug-ins, how should preferences be handled?

ruspl-afed commented 3 years ago

I assume it should be at "org.eclipse.embedcdt.managedbuild.cross.riscv.core" where preferences are stored and handled If preferences are UI-specific they should live at "org.eclipse.embedcdt.managedbuild.cross.riscv.ui"

ilg-ul commented 3 years ago

Yes, I was afraid you'll say this :-(

Unfortunately this complicates the compatibility logic used to read preferences.

As of now, I patched the preferences class to retry on failed reads, so if a read with org.eclipse.embedcdt.xxx fails, I retry with ilg.gnumcueclipse.xxx.

Once we split the preferences into separate plug-ins, we probably have to add a map to define the compatibility ID for each new ID.

ruspl-afed commented 3 years ago

Actually we can use any namespace to store the preferences

ilg-ul commented 3 years ago

We can, but should we? I think that the convention to use the plug-in name is fine, and we should follow it.

For compatibility reason we will read from both IDs. I'll handle these details after you finish to split the classes. I'll probably add an array of IDs to the preferences store manager class, and it'll iterate them.

ruspl-afed commented 3 years ago

Closing this one, now it is a part of #441