-
Notifications
You must be signed in to change notification settings - Fork 50
iTools packager: license files and misc files #3654
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Damien Jeandemange <[email protected]>
Signed-off-by: Damien Jeandemange <[email protected]>
Signed-off-by: Damien Jeandemange <[email protected]>
|
I think that the issue here is more that the "child project" (powsybl-distribution in your example) does not have a
|
Hi @rolnico The issue for powsybl-distribution is that current implementation looks in parent folder always, so even if these files were added, they would not be found at all. The warning in GitLab CI is But what can be added to powsybl-distribution is: |
Exactly, that's why I said that fixing the paths could be interesting. |
|
Signed-off-by: Damien Jeandemange <[email protected]>
Signed-off-by: Damien Jeandemange <[email protected]>
Hi @rolnico , done as per your suggestion above, let me know |
rolnico
left a comment
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.
I would make some changes: if the user specifies a license or 3rd party file, I think we should not look for a default file if the users file does not exist.
What do you think of that?
| <licenseFile>myLic.txt</licenseFile> | ||
| <thirdPartyFile>my3rdParties.txt</thirdPartyFile> |
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.
In this case, the files do not exist, but the standard files exist in the parent folder and we'll use those files. Is that the expected behavior? Or should we throw an exception when files with the expected names are not found?
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.
was intended, changed as you wanted.
you may change further if needed.
| private void addLicenseFiles(Path packageDir) { | ||
| // List of the license files to copy | ||
| List<String> licenseFiles = Arrays.asList("LICENSE.txt", "THIRD-PARTY.txt"); | ||
| Path projectRoot = Path.of(project.getBasedir().getPath()); |
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.
| Path projectRoot = Path.of(project.getBasedir().getPath()); | |
| Path projectRoot = project.getBasedir().toPath(); |
| List<Path> candidateLicenseFiles = Stream.of(licenseFile, "LICENSE.txt", "../LICENSE.txt") | ||
| .filter(Objects::nonNull).map(projectRoot::resolve).toList(); |
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.
| List<Path> candidateLicenseFiles = Stream.of(licenseFile, "LICENSE.txt", "../LICENSE.txt") | |
| .filter(Objects::nonNull).map(projectRoot::resolve).toList(); | |
| List<Path> candidateLicenseFiles = getFilePathList(licenseFile, "LICENSE", projectRoot); |
| List<Path> candidateThirdPartyFiles = Stream.of(thirdPartyFile, "THIRD-PARTY.txt", "../THIRD-PARTY.txt") | ||
| .filter(Objects::nonNull).map(projectRoot::resolve).toList(); | ||
| addLicenseFile(packageDir, candidateThirdPartyFiles); | ||
| } |
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.
I would suggest a change, to only look for default files if no license/3rd party file was given by the user:
| List<Path> candidateThirdPartyFiles = Stream.of(thirdPartyFile, "THIRD-PARTY.txt", "../THIRD-PARTY.txt") | |
| .filter(Objects::nonNull).map(projectRoot::resolve).toList(); | |
| addLicenseFile(packageDir, candidateThirdPartyFiles); | |
| } | |
| List<Path> candidateThirdPartyFiles = getFilePathList(thirdPartyFile, "THIRD-PARTY", projectRoot); | |
| addLicenseFile(packageDir, candidateThirdPartyFiles); | |
| } | |
| private List<Path> getFilePathList(String fileName, String defaultFileNameBase, Path projectRoot) { | |
| if (fileName != null) { | |
| return List.of( | |
| projectRoot.resolve(fileName), | |
| projectRoot.resolve("..").resolve(fileName) | |
| ); | |
| } | |
| // Search for default names | |
| return Stream.of("", ".txt") | |
| .flatMap(ext -> Stream.of( | |
| projectRoot.resolve(defaultFileNameBase + ext), | |
| projectRoot.resolve("..").resolve(defaultFileNameBase + ext) | |
| )) | |
| .toList(); | |
| } |
| FileUtils.deleteDirectory(target2); // cleanup | ||
| String target = project.getBuild().getDirectory(); | ||
| assertTrue(new File(target, DEFAULT_PACKAGE_NAME + ".zip").exists()); | ||
| assertTrue(new File(target, DEFAULT_PACKAGE_NAME + "/LICENSE.txt").exists()); |
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.
| assertTrue(new File(target, DEFAULT_PACKAGE_NAME + "/LICENSE.txt").exists()); | |
| assertFalse(new File(target, DEFAULT_PACKAGE_NAME + "/LICENSE.txt").exists()); |
if you change also in the test pom.xml
| </executions> | ||
| <configuration> | ||
| <licenseFile>myLic.txt</licenseFile> | ||
| <thirdPartyFile>my3rdParties.txt</thirdPartyFile> |
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.
| <thirdPartyFile>my3rdParties.txt</thirdPartyFile> |
remove this to test the case where only one file is specified (and since it does not exist, the test is modified too)
Signed-off-by: Damien Jeandemange <[email protected]>
|
rolnico
left a comment
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.
LGTM



Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
No
What kind of change does this PR introduce?
Bugfix & Feature
Rework of #3443.
What is the current behavior?
What is the new behavior (if this is a feature change)?
In case no file could be found, a log indicates all attempted locations for license and third party.
Does this PR introduce a breaking change or deprecate an API?
If yes, please check if the following requirements are fulfilled
What changes might users need to make in their application due to this PR? (migration steps)
Other information: