search

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

Move some classes to the org.eclipse.embedcdt.internal namespace #451

Closed ilg-ul closed 3 years ago

ilg-ul commented 3 years ago

In order to simplify the public API, it would be nice to move some classes to the org.eclipse.embedcdt.internal namespace.

Alexander tried to summarise the subject in https://github.com/eclipse-embed-cdt/eclipse-plugins/pull/441#issuecomment-735445878

I prepared a new branch develop-internal an try to move some of the classes accompanied by object IDs, but I'm not sure I'm not messing things.

ilg-ul commented 3 years ago

Jonah @jonahgraham, I think we're done now.

Could you take a look at the result in develop-internal and let me know if the way I renamed the UI classes related to preferences/properties/views/perspectives is ok? If so, I'll merge it and it'll be the final 6.0.0.

Alexander @ruspl-afed, does this change match your expectations? Do you have any suggestion on other classes to be moved to the internal namespace? If necessary, we have a few more days for last minute changes.

BTW, I understand how moving classes to the non-exported internal namespace hides them from curious integrators, but for the objects that have IDs, this is not 100% efficient, since they can use the IDs without importing anything.

Actually there is one such case in the project, the codered perspective makes use of the peripheral registers view.

Am I missing something?

ilg-ul commented 3 years ago

BTW, for testing I used the CPP package from the new RC1, plus the new plug-ins from https://download.eclipse.org/embed-cdt/builds/develop-internal/p2/.

I testes STM32F4 and RISC-V projects, with QEMU, OpenOCD and J-Link.

jonahgraham commented 3 years ago

I made a line comment on https://github.com/eclipse-embed-cdt/eclipse-plugins/commit/bf1f70493e8caccdfd3fbbb3e60ba8c352892ba5 that says:

The name "internal" is just a convention - it needs to be marked as x-internal for the API tooling in Eclipse to warn users of the API that it is internal.

I added a few commits to mark a bunch of the code as internal. See commits, their commit messages have additional details

Note that e3676409eb may be partially questionable - we can discuss whether to use the export all package or not rule. The Eclipse Platform recently changed the recommendations. I personally strongly object to that, and embedcdt is not bound in any ways by their rules. The current rules for Platform are https://wiki.eclipse.org/Export-Package the old policy was https://wiki.eclipse.org/index.php?title=Export-Package&oldid=237317

The key part of the original policy for me was "The community's desire/need for the freedom to access non-API is overwhelming. In fact, this forms a critical part of the Eclipse ecosystem by allowing experimentation and investigation." - but some people now find that it caused extenders to use the not exported API instead of contributing back. Of course for years I was that community and my personal interest and success with Eclipse ecosystem was because of that experimentation and investigation.

Anyway, feel free to revert e3676409eb, but I would keep at least some of the changes - such as marking the exported packages with internal in their name as x-friends, for example https://github.com/eclipse-embed-cdt/eclipse-plugins/blob/e3676409eb746c0853992b04808882b214f042cf/plugins/org.eclipse.embedcdt.managedbuild.cross.ui/META-INF/MANIFEST.MF#L30-L35

ilg-ul commented 3 years ago

Jonah, since you definitely know these things better than I do, I suggest we keep these commits.

I hope that we'll not have to annoy the users with a new major release soon.

Alexander @ruspl-afed, since you triggered this discussion, please confirm that you're ok with the current status, or suggest further changes.

By Tue Dec 8th I plan to merge the code to the master branch and commit the final 6.0.0 release.

ilg-ul commented 3 years ago

Code merged into develop on Dec 7th.

ruspl-afed commented 3 years ago

Sorry, there were no room to review this work. In any case the sources for 6.0.0 are prepared and we can potspone this discussion for a while.

ilg-ul commented 3 years ago

we can potspone this discussion for a while.

sure.

just that we'll no longer have the freedom to move things around, we have to stick to this new API.