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

Use external dependencies from Maven #694

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

schuessf
Copy link
Contributor

@schuessf schuessf commented Nov 15, 2024

Currently, there are many different ways how external dependencies are used in Ultimate:

However, I presume the first solution is the most optimal one, as one can simply add external dependencies without any changes on some servers (like the second solution) or without copying any files into the source files of Ultimate that can be also easily updated (just change the version number in the target file). Therefore I attempted to only use Maven dependencies, where possible, there are currently some exceptions:

  • Library-Jung and Library-ApacheCommonsCLI were patched to our needs (e.g. 8b6c0bf), so we cannot replace them with external dependencies without.
  • While Eldarica is available on Maven, Library-CHC/Eldarica-assembly-2.0.8.jar contains actually more dependencies.
  • BA_SharedJARs contains multiple JARs, but I am not sure, if they can be replaced with Maven dependencies or where these JARs are actually needed.

There is still one open issue with this PR: Building works totally fine for me using Ultimate_E4.32_Java21.target as a target file, however if I use Ultimate_E4.32_Java21_Win32.target the external dependenices cannot be resolved (e.g. org.apache.commons.lang3), even though Ultimate_E4.32_Java21.target is included in the target file. (This was fixed by adding the dependencies to includeBundles)

@danieldietsch
Copy link
Member

  • Did you per chance compare the size of our deployments before/after this change?
  • Would it work with the platform-dependent target files if you added the new libraries as included bundles in the platform-dependent files?

@schuessf
Copy link
Contributor Author

Did you per chance compare the size of our deployments before/after this change?

It is slightly smaller, e.g. 117MB instead of 119MB for the Debug-E4

Would it work with the platform-dependent target files if you added the new libraries as included bundles in the platform-dependent files?

Yes, it does work, see 171987b.

Copy link
Contributor

@maul-esel maul-esel left a comment

Choose a reason for hiding this comment

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

👍 thanks for taking care of this.

I agree we should keep pushing on this in further PRs for the other JARs, and perhaps we can also adapt some code such that we do not need a patched version e.g. of ApacheCommonsCLI?

@danieldietsch
Copy link
Member

[...] and perhaps we can also adapt some code such that we do not need a patched version e.g. of ApacheCommonsCLI?

For ApacheCommonsCLI rather not. We have many modifications to that part of Apache Commons, which we use for our command line interface. Even moving it to a separate repo would be counter-productive, as changing and testing that when, e.g., we add a new option type, would be much more tedious.

@schuessf
Copy link
Contributor Author

I agree we should keep pushing on this in further PRs for the other JARs

I agree, this PR was just meant as a start, where I tried to simply use unmodified Maven dependencies where possible, without changing any code. There are still some dependencies left, where this is not possible, because they are not on maven (petruchio.jar, czt_1_5_0_bin.jar, JavaCup) or because we use some patched version (Library-Jung). Maybe we could simply add them to our P2 repository in a future PR.

@schuessf
Copy link
Contributor Author

[...] and perhaps we can also adapt some code such that we do not need a patched version e.g. of ApacheCommonsCLI?

For ApacheCommonsCLI rather not. We have many modifications to that part of Apache Commons, which we use for our command line interface. Even moving it to a separate repo would be counter-productive, as changing and testing that when, e.g., we add a new option type, would be much more tedious.

It looks like, we adapted ApacheCommonsCLI only in four commits (804d6ba, 2c29ed9, ff91644, 8b6c0bf). Instead of changing the source files, we could also use the unpatched versions and for example extend the classes there to our needs (if this is possible). But again: This is not really urgent 😉

@maul-esel
Copy link
Contributor

For CZT, we should talk to the ReqAnalyzer people -- when asking @Langenfeld, he was not sure whether it is still needed.

@danieldietsch
Copy link
Member

[...] and perhaps we can also adapt some code such that we do not need a patched version e.g. of ApacheCommonsCLI?

For ApacheCommonsCLI rather not. We have many modifications to that part of Apache Commons, which we use for our command line interface. Even moving it to a separate repo would be counter-productive, as changing and testing that when, e.g., we add a new option type, would be much more tedious.

It looks like, we adapted ApacheCommonsCLI only in four commits (804d6ba, 2c29ed9, ff91644, 8b6c0bf). Instead of changing the source files, we could also use the unpatched versions and for example extend the classes there to our needs (if this is possible). But again: This is not really urgent 😉

My rule of thumb would be:

  • Unmodified libraries with "good" autogenerated manifests should go in the target files.
  • Unmodified libraries where we have to create a manifest by ourselves or those that are not available via Maven should go in the p2 dependency repo.
  • Modified libraries should be part of our repo, because it simplifies development.

For ApacheCommonsCLI I am not sure if we want to deal with changes in upstream as long as it does what we want.

@danieldietsch
Copy link
Member

For CZT, we should talk to the ReqAnalyzer people -- when asking @Langenfeld, he was not sure whether it is still needed.

CZT is only needed for Library-PEA, and there for the Z support. I am pretty sure that they dont use that, but they would need to remove it from Library-PEA. Question is: do they want to do that soonish, or would it be ok if someone else does it?
It would be nice to get rid of it.
@Langenfeld @henkele ?

Copy link
Member

@bahnwaerter bahnwaerter left a comment

Choose a reason for hiding this comment

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

Thanks @schuessf for starting this work to beautify the code base and its dependencies. I particularly like that we can now integrate libraries like Trove cleanly.

@bahnwaerter
Copy link
Member

For CZT, we should talk to the ReqAnalyzer people -- when asking @Langenfeld, he was not sure whether it is still needed.

CZT is only needed for Library-PEA, and there for the Z support. I am pretty sure that they dont use that, but they would need to remove it from Library-PEA. [...]

That was also the statement of @jhoenicke that he only needed this back then, before the PEA-to-Boogie translation was available. Nowadays it is no longer used and it seems as if the CZT project is dead.

On Maven, only Eldarica 2.1 is available, where a few packages where moved and the API slightly changed.
The previously used JAR contains Eldarica, Princess and Scala, therefore we need these three Maven dependencies.
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

Successfully merging this pull request may close these issues.

4 participants