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

Fixing JT-173 (backport of modified version of PR made by phax) #287

Merged
merged 1 commit into from
Aug 14, 2023

Conversation

laurentschoelens
Copy link
Collaborator

Phax does this PR which doesn't break build and fixes his issue #173
Without it, he forked the project and maintains it (with jakarta support).

@laurentschoelens
Copy link
Collaborator Author

Added UT for IOUtils new function.
Made some changes in initial function of phax since issues discovered while adding UT.

@mattrpav
Copy link
Collaborator

mattrpav commented Aug 8, 2023

Do we have a jaxb-maven project to confirm it works? Seems his issue was about ordering of schemas.

@mattrpav mattrpav self-assigned this Aug 8, 2023
@mattrpav mattrpav added this to the 2.0.4 milestone Aug 8, 2023
@laurentschoelens
Copy link
Collaborator Author

Do we have a jaxb-maven project to confirm it works? Seems his issue was about ordering of schemas.

All of existings jaxb-maven projects tests confirm that there is no regression.
This PR is also based on the original PR of phax but with better sort handling since we could have non wanted sorts items (initial PR tested path endsWith includes items path, which may me wrong).

The main goal of this PR is to have files resolution as close as includes items even if files are not returned in a consistent way across build platforms.

@mattrpav
Copy link
Collaborator

mattrpav commented Aug 8, 2023

Do we have a test project that uses "*" to references schemas and have schemas in a non-matching folder to make sure they do not get included?

User's original dynamic list (issue showed up as resolution when really wide):

              <schemaIncludes>
                <schemaInclude>common/*.xsd</schemaInclude>
                <schemaInclude>maindoc/*.xsd</schemaInclude>
              </schemaIncludes>

The goal of this fix is to ensure that the order of files specified in included
configuration section is respected and used in plugin, no matter how plexus
returns them by DirectoryScanner since order is not consistent.

See issue codehaus-plexus/plexus-utils#70 in plexus-utils
@laurentschoelens laurentschoelens changed the title Fixing JT-173 (backport of initial PR made by phax) Fixing JT-173 (backport of modified version of PR made by phax) Aug 9, 2023
@laurentschoelens
Copy link
Collaborator Author

Reworked on the PR
The main point is DirectoryScanner in plexus-utils does not return files in a consistent way
So here, we do take the result of DirectoryScanner files, and reorder them strictly according to includes configuration section in the pom.

We use the same patternmatcher as plexus-utils does to check if returned file does match each include pattern in configuration order to sort them this way.
Added more UT to validate the fix.

Copy link
Collaborator

@mattrpav mattrpav left a comment

Choose a reason for hiding this comment

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

LGTM

@mattrpav mattrpav merged commit f429fa2 into highsource:master Aug 14, 2023
3 checks passed
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.

2 participants