fix: resolving property references in xml files #163
Conversation
|
Since the recent release tests are no longer running, I'll fix this so that i can be reviewed. |
… report failing to generate
Spezialissimo
changed the title
|
Errors have been fixed |
src/sbt-test/dumpLicenseReport/property-reference-replace/project/build.properties
Outdated
Show resolved
Hide resolved
...Report/property-reference-replace/test-library/com/example/fake-lib/1.0.0/fake-lib-1.0.0.pom
Outdated
Show resolved
Hide resolved
|
@Spezialissimo Need to also run a scalafmt |
|
So far the PR looks good, the only thing I am concerned about is the fallback logic for detecting the ivy properties. I am somewhat sure that sbt also has logic for doing the same just that its private and not accessible from sbt plugins, would it be possible to do some research here? @raboof maybe you want to look into this as well? @eed3si9n @dwijnand If its not too much work maybe you can provide input here (regarding how xml property substitution works with ivy). I think you two know more about ivy than anyone else in the Scala space |
| case (Some(node), label) => (node \ label).headOption | ||
| case _ => None | ||
| } | ||
| finalNode.map(_.text.trim).getOrElse("") |
There was a problem hiding this comment.
This default might make it harder to figure out 'downstream' that there is something missing/redacted here. I'm OK with it, though.
There was a problem hiding this comment.
Might be good to add a logger warning here, @Spezialissimo read https://www.scala-sbt.org/1.x/docs/Howto-Logging.html#Log+messages+in+a+task on how to log in sbt
There was a problem hiding this comment.
@mdedetrich Thanks! I'll take a look
There was a problem hiding this comment.
Also its possible for trim itself to return an empty string "" so you need to handle that case as well. In other words if at any point an empty string is resolved (either because Optional value is empty and it goes to the .getOrElse("") part or there is an empty String value after .trim) we should log that as a warn
There was a problem hiding this comment.
@mdedetrich @raboof
I've pushed the changes addressing your suggestions, thanks a lot!
Let me know if there is anything else
iirc sbt resolves these property substitutions (even when Ivy is being used) so it must be done somewhere??? |
|
@mdedetrich Let me know if there is something else I should look into for this PR, otherwise it would be great if it could be merged and released |
One minor nit and then ill approve/merge |
|
@Spezialissimo This is now being published as 1.9.0 to maven https://github.com/sbt/sbt-license-report/actions/runs/17974075180/ |
|
Thank you @mdedetrich and @raboof for taking a look into this! |
3 participants
PROBLEM
Ivy does not replace property references in xml files. That means that when they are present, we might get a report that looks like this:
I noticed that in some (rare) cases the original xml file only contains the url as info about the license. For this reason, I thought it would have been great if we were able to replace such references before creating the report.
SOLUTION
The current implementation uses a best effort approach.
If it finds any
${}it will try to find the xml file that ivy is retrieving information from. If it's able to find the file, it will try to find the replacement and update the UpdateReport object. If any of this fails, nothing happens and the references don't get replaced.I'm not an expert of how the information gets originally retrieved, for this reason it will check, in order:
.pomfile with same path and name than the.jarfile..xml.originalfile present two directories up from the.jarfile..xmlfile present two directories up from the.jarfile.As these are the cases I found in my experience. If you know of way to improve this search please let me know.
Also, I added a test to check that the replacement is working. I want to reiterate that i'm not an expert of the various edge cases so let me know if you think the tests can be improved.
Additionally, other cases of failures in creating the URI object for the homepage field are now handled, taking in consideration that the ".toUri" function seems to not disclose every kind of exception it can throw.