Skip to content

Conversation

@wadoon
Copy link
Member

@wadoon wadoon commented May 24, 2024

This brings some clean-ups of the #3451 branch to main, without adding module-info.java files.

Features

  • distinct packages for each (gradle) module
  • fixing service loader files

Will break binary and source compatibility.

Future:

  • module-info.java is currently not possible due to stringtemplate4.
  • We might want to set an module name in the manifest to support auto modules.

@wadoon wadoon mentioned this pull request May 24, 2024
3 tasks
@wadoon wadoon force-pushed the weigl/pckgreworked branch from 0b3f5bd to 29e5cce Compare May 24, 2024 21:45
@wadoon wadoon changed the title Weigl/pckgreworked Package structured reworked for sealed packages May 24, 2024
@wadoon wadoon marked this pull request as ready for review May 26, 2024 15:51
@wadoon wadoon self-assigned this May 26, 2024
@wadoon wadoon added the 🛠 Maintenance Code quality and related things w/o functional changes label May 26, 2024
@wadoon wadoon added this to the v2.14.0 milestone May 26, 2024
@wadoon wadoon requested a review from WolframPfeifer June 6, 2024 09:02
@wadoon wadoon force-pushed the weigl/pckgreworked branch from 7200b6a to 87c7a64 Compare June 24, 2024 13:21
@wadoon wadoon force-pushed the weigl/pckgreworked branch from 4cad585 to a2b1d9a Compare November 17, 2024 22:10
@KeYProject KeYProject deleted a comment from sonarqubecloud bot Jan 25, 2025
@KeYProject KeYProject deleted a comment from sonarqubecloud bot Jan 25, 2025
@wadoon wadoon enabled auto-merge January 25, 2025 20:46
Copy link
Member

@WolframPfeifer WolframPfeifer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I think it is a good idea to have disjoint packages for our (Gradle) submodules (i.e. key.core, key.ui, ...), even if we do not really use the module system at the moment.

Disabling the Ctrl+C signal handling is also fine, it was an experiment and apparently only worked correctly on some machines.

However, unfortunately there are the following blockers:

  • Gradle task for GUI is not working, since build.gradle was not updated.
  • Proof caching is completely disabled (?)

For a refactoring like this, I think you should have checked that everything works (at least a sanity check, i.e. running the KeY GUI once and see if the extensions are still there). In my opinion, this is not the task of the reviewer.

Minor remarks and open questions:

  • Can you please update/complete the PR description? Otherwise, it is difficult to understand what this PR does exactly without looking into the code. Might also be relevant for the changelog ...
  • I think the package name de.uka.ilkd.key.ui.core does not really make sense. Also, the separation between this, de.uka.ilkd.key.ui, and de.uka.ilkd.key.gui is not clear. If we are at restructuring, we should think of this as well.
  • Is there any check/guarantee that the packages are actually disjoint now? It is really not obvious ...
  • Can we prevent people from destroying disjointness accidentally in the future (for example, by adding a file in key.ui to package de.uka.ilkd.key.core)?

Thanks for the refactoring! If these points are resolved, it can be merged in my opinion.

de.uka.ilkd.key.gui.nodeviews.ShowHashcodesExtension
de.uka.ilkd.key.gui.LogView
de.uka.ilkd.key.gui.plugins.javac.JavacExtension
de.uka.ilkd.key.gui.plugins.caching.CachingExtension
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caching extension is not available in the GUI any more. I think this is the reason ...

@WolframPfeifer
Copy link
Member

Btw.: What do you mean by the label breaks API?

@wadoon
Copy link
Member Author

wadoon commented Mar 10, 2025

Btw.: What do you mean by the label breaks API?

Breaks API indicates that a PR influences KeY's API and ABI. Hence, every downstream library, e.g., KeY4Eclipse, needs to be adopted---in this case, by changing the imports.

@wadoon wadoon force-pushed the weigl/pckgreworked branch from d598004 to 39eed3f Compare May 10, 2025 15:42
@WolframPfeifer WolframPfeifer added the Reviewer Feedback Feedback from the review needs to be addressed label May 14, 2025
@wadoon wadoon closed this Jun 14, 2025
auto-merge was automatically disabled June 14, 2025 20:07

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaks API 🛠 Maintenance Code quality and related things w/o functional changes Reviewer Feedback Feedback from the review needs to be addressed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants