hyperxpro / Brotli4j

Brotli4j provides Brotli compression and decompression for Java.
Apache License 2.0
105 stars 35 forks source link

Add back previously deleted profiles #84

Closed ichttt closed 1 year ago

ichttt commented 1 year ago

Should fix #82 Not quite sure yet why I removed it but doesn't seem to cause any issues

hyperxpro commented 1 year ago
Error: ] The projects in the reactor contain a cyclic reference: Edge between 'Vertex{label='com.aayushatharva.brotli4j:brotli4j:1.9.0'}' and 'Vertex{label='com.aayushatharva.brotli4j:native-osx-x86_64:1.9.0'}' introduces to cycle in the graph com.aayushatharva.brotli4j:native-osx-x86_64:1.9.0 --> com.aayushatharva.brotli4j:brotli4j:1.9.0 --> com.aayushatharva.brotli4j:native-osx-x86_64:1.9.0 @ 
Error:  The projects in the reactor contain a cyclic reference: Edge between 'Vertex{label='com.aayushatharva.brotli4j:brotli4j:1.9.0'}' and 'Vertex{label='com.aayushatharva.brotli4j:native-osx-x86_64:1.9.0'}' introduces to cycle in the graph com.aayushatharva.brotli4j:native-osx-x86_64:1.9.0 --> com.aayushatharva.brotli4j:brotli4j:1.9.0 --> com.aayushatharva.brotli4j:native-osx-x86_64:1.9.0 -> [Help 1

:(

ichttt commented 1 year ago

Ah ok I found the reason why: It introduces a cyclic reference when buildung due to the submodules referencing the main module...

ichttt commented 1 year ago

Well, the only way to properly fix this is probably by moving the service interface into its own module and adding a dep from brotli4j and the natives to that module. That would also allow me to remove brotli4j-tests again and merge it into the main brotli4j module. Doesn't sound optimal, but it is the only thing I can think of that should work. I'll try to make that change later today.

ichttt commented 1 year ago

Should be good now. Rudimentary testing on my linux machine shows everything is still working.

hyperxpro commented 1 year ago

Your change breaks API now. We cannot rename packages.

ichttt commented 1 year ago

Technically it does, but I dont't see any other way to split it. As the new jar needs to have its own package for module separation purposes. I also don't think anybody really uses this interface (also least a quick search on grep.app shows no results apart from this repo).

hyperxpro commented 1 year ago

Please find some other way to fix this.

hyperxpro commented 1 year ago

I will remove modules support. It is better to break modules support than breaking API.

ichttt commented 1 year ago

Are we talking about the same breaking change? Cause the only breaking change I am seeing is the change of moving the BrotliNativeProvider, which is a class I added for module support, to a different package. This class is really not something brotli4j users should use and has only existed since the 1.9.0 release. If you are still not in favor of moving its package, I have a few ideas:

  1. Keep the old class in the Brolti4j main module, deprecate it and no longer search for services implementing it
  2. Keep the old class in the Brolti4j main module, deprecate it and add code that searches for services implementing it
  3. Keep the old class and hack around the issue by making the service dep a dep with the provided scope on the natives and remove it on the main brotli4j module so it loads from brotli4j module on runtime but uses the service module on build time. This would require the class to be duplicated though, which is not optimal.

I would still argue that moving the package of this class, even if it is not a major version change and it is technically a breaking change, is an ok thing to do. But I'll leave that up to you.

hyperxpro commented 1 year ago

By breaking change, I mean changing package names completely. This looks fine. I will try to improve if I can else I will cut release.

ichttt commented 1 year ago

Yeah that was only the tests and due to bad refactoring. I though you meant the service interface :)