-
Notifications
You must be signed in to change notification settings - Fork 165
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
GH-2710 module structure reorganization #3225
Conversation
Looks like our use of multithreading for the maven build does not agree with the new modular structure. Verified locally that a single-threaded build succeeds. Possibly caused by my relying on transitive dependencies for including libraries like commons-annotations in some places (via rdf4j-model). I'll see if more explicit dependencies can fix this, otherwise may temporarily disable multithreaded builds in our Github actions. |
|
7a9fd30
to
7deb0a7
Compare
It's a bit big for me to review. Does this affect any of the ShaclSail code in any way? Could you confirm that this builds with the following method?
|
import org.eclipse.rdf4j.IsolationLevel; | ||
import org.eclipse.rdf4j.RDF4JException; | ||
import org.eclipse.rdf4j.common.exception.RDF4JException; | ||
import org.eclipse.rdf4j.common.transaction.IsolationLevel; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to move IsolationLevel and RDF4JException to common
*/ | ||
@Deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the method could be removed altogether, instead of just being deprecated ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably could, but I didn't want to make that decision in this PR.
Yeah, I'd be happy with just checking out the branch and giving it a quick smoke test, eyeballing a few specific changes. I'd like this merged in soon so that we can shake out any problems as we go.
A little. There's some refactoring with the exception classes.
I just ran that (except I used I'll fix those in a separate PR, then rebase this one and try again. |
bfa1829
to
5817f22
Compare
Ran the offline build test again with the dependency fixes merged in, all green. |
Unless there are major objections I'll merge this tomorrow morning - I'd like to get this in as soon as possible to minimize the risk of merge conflicts. Of course, I'll be on hand to help out on any other PRs that get conflicts because of this. |
GitHub issue resolved: #2710
Briefly describe the changes proposed in this PR:
This is quite a large refactor, but it gives us greater flexibility, cleans up a few old things, and sets us up to make followups like #1494 (each package in a single module) easier to achieve.
PR Author Checklist (see the contributor guidelines for more details):
mvn process-resources
to format from the command line)