[GitHub] ant-ivy pull request #74: Fix IVY-982 by removing negated entries from wildc...
Github user aprelev closed the pull request at: https://github.com/apache/ant-ivy/pull/74 --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #74: Fix IVY-982 by removing negated entries from wildcard bin
Github user aprelev commented on the issue: https://github.com/apache/ant-ivy/pull/74 Do you think overhead decrease achieved by using `singletonList()` outweighs breaking the pattern of `List` initialisations in test body? This is my second week with Java, these subtle differences are new to me. --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #74: Fix IVY-982 by removing negated entries from wildcard bin
Github user aprelev commented on the issue: https://github.com/apache/ant-ivy/pull/74 I replaced `Arrays.equals()` with `List.containsAll()`, as you suggested; thank you. --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy pull request #74: Fix IVY-982 by subtracting negated entries from wi...
GitHub user aprelev opened a pull request: https://github.com/apache/ant-ivy/pull/74 Fix IVY-982 by subtracting negated entries from wildcard bin ### Issue Configurations negation (exclusion) as in `conf="*, !foo->@"` does not work, This issue is reported in [IVY-982](https://issues.apache.org/jira/browse/IVY-982) and [IVY-1547](https://issues.apache.org/jira/browse/IVY-1547). ### Why does it happen? Resolve engine silently disrespects negation on the left part of maps-to operator because the exclusion was not implemented. When parsing dependency, e.g. `conf="*, !foo â bar1; foo â bar2, bar3; % â bar4"`, all dependency configurations are collected into bins (map entries): - *all-wildcard bin* with all configurations required for `*` superset, such as `bar1` in bin `*`; - *others-wildcard bin* with all configurations required for `%` superset, such as `bar4` in bin `%`; - *explicit bins* for all explicit mappings, such as `bar2` and `bar3` in bin `foo`, and `bar1` in bin `!foo`. Resolving list of dependency configurations required for some target configuration `X` is done as follows: 1. All configurations from `X`'s *explicit bin* [are added](https://github.com/apache/ant-ivy/blob/89583444040dc5423bb143435f23ae0814f24542/src/java/org/apache/ivy/core/module/descriptor/DefaultDependencyDescriptor.java#L347). 2. All configurations from *others-wildcard bin* [are added](https://github.com/apache/ant-ivy/blob/89583444040dc5423bb143435f23ae0814f24542/src/java/org/apache/ivy/core/module/descriptor/DefaultDependencyDescriptor.java#L350) in case `X`'s *explicit bin* is empty. 3. All configurations from *all-wildcard bin* [are added](https://github.com/apache/ant-ivy/blob/89583444040dc5423bb143435f23ae0814f24542/src/java/org/apache/ivy/core/module/descriptor/DefaultDependencyDescriptor.java#L358). Note that explicit bins for negated target configurations *are not referenced, and thus silently ignored*. This fix introduces fourth step: 4. All configurations from `!X`'s *explicit bin* are removed. You can merge this pull request into a Git repository by running: $ git pull https://github.com/aprelev/ant-ivy issue-ivy-1547-982 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/ant-ivy/pull/74.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #74 commit 0c2c826e4c316fdcf42fd890097edef478b692a3 Author: Aprelev Arseny Date: 2018-08-06T14:21:21Z Fix IVY-982 by subtracting negated entries from wildcard bin --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #73: IVY-1104 Include attributes qualifiers in the XML report
Github user aprelev commented on the issue: https://github.com/apache/ant-ivy/pull/73 Why doesn't it show as **merged** though? --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy pull request #73: IVY-1104 Include attributes qualifiers in the XML ...
Github user aprelev closed the pull request at: https://github.com/apache/ant-ivy/pull/73 --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy pull request #73: IVY-1104 Include attributes qualifiers in the XML ...
Github user aprelev commented on a diff in the pull request:
https://github.com/apache/ant-ivy/pull/73#discussion_r207675516
--- Diff: src/java/org/apache/ivy/plugins/report/XmlReportParser.java ---
@@ -193,16 +193,8 @@ public void startElement(String uri, String localName,
String qName,
String branch = attributes.getValue("branch");
String revision = attributes.getValue("revision");
Map extraAttributes = new
HashMap<>();
--- End diff --
It was, until I reused code for attributes extraction from
`ExtendableItemHelper`.
---
-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #73: IVY-1104 Include attributes qualifiers in the XML report
Github user aprelev commented on the issue: https://github.com/apache/ant-ivy/pull/73 Okay, I've implemented robust ASN.1-like encoding as ``` prefix* qualifierLength separator qualifier* separator* name ``` such as `extra-3.foo-bar` for `foo:bar`, and `extra-0.foo` for `foo`, which is up for review. P.S. Also, point me in the right direction regarding XSLT. --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #73: IVY-1104 Enable XML report parser to produce qualified ex...
Github user aprelev commented on the issue: https://github.com/apache/ant-ivy/pull/73 To be honest, I do not like the idea of imposing implicit requirements on naming of namespaces. Escaping dashes in prefixes seems more robust and error-prone given correct codec is implemented. I will look into that and update this PR accordingly. --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #73: IVY-1104 Enable XML report parser to produce qualified ex...
Github user aprelev commented on the issue: https://github.com/apache/ant-ivy/pull/73 > Using a simple '-' as a separator may cause trouble if somebody decides to use a namespace prefix with '-'. I don't actually see a problem with that: we can - either escape dashes with some other character, like `.` (`-ns-:-foo.` â `extra-.-ns.--.-foo..`), - or replace them with `` (`-ns-:-foo.` â `extra-ns-foo.`). --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #73: IVY-1104 Enable XML report parser to produce qualified ex...
Github user aprelev commented on the issue: https://github.com/apache/ant-ivy/pull/73 @twogee, why don't we just stick with deterministic encoding like ```Java extra- // e.g. extra-e:foo, extra-m:classifier, extra-foo ``` since both `'-'` and `':'` are allowed name characters? This would also incur minimum changes to the codebase: literally passing `::getQualifiedExtraAttributes()` to `XmlReportWriter::extraToString()` so qualifiers can be picked up by `XmlReportParser::startElement()` automatically. Or, if you want to avoid using `':'` in the report (if original `extra-` prefix was supposed to replace the namespace, as you guessed it was), we could use another deterministic encoding like ```Java extra-- // e.g. extra-e-foo, extra-m-classifier, extra--foo ``` which would require additional encoding and decoding overhead in both XML report parser and writer, but otherwise imply no restrictions on naming? Also, in the name of code clarity and with respect to single responsibility principle, in my opinion, it would be better if `ExtendableItemHelper` handled both encoding and decoding of attributes (as of now, encoding is handled by `XmlReportWriter::extraToString()`). --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...
Github user aprelev commented on the issue: https://github.com/apache/ant-ivy/pull/73 By the way, found [issue IVY-1104](https://issues.apache.org/jira/browse/IVY-1104) in your issue tracker, seems to be the case. --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...
Github user aprelev commented on the issue:
https://github.com/apache/ant-ivy/pull/73
As of now, two sets of dependencies are constructed in a different ways:
- *currently resolved dependecies* are produced by Ivy resolve engine, with
**both qualified and unqualified attributes** (`super.qualifiedExtraAttributes
!= super.extraAttributes`), and
- *previously resolved dependencies* are parsed from last report by
`XmlReportParser`, with **unqualified attributes only**
(`super.qualifiedExtraAttributes == super.extraAttributes`).
Set comparison at `ConfigurationResolveReport::checkIfChanged()` invokes
`ModuleRevisionId::equals()` for individual dependencies comparison, which
compares `super.qualifiedExtraAttributes` maps as in:
```Java
@Override
public boolean equals(Object obj) {
...
return other.getRevision().equals(getRevision())
&& !(other.getBranch() == null && getBranch() != null)
&& !(other.getBranch() != null &&
!other.getBranch().equals(getBranch()))
&& other.getModuleId().equals(getModuleId())
&&
other.getQualifiedExtraAttributes().equals(getQualifiedExtraAttributes()); //<
here
}
```
which, obviously, yields a `false`, since keys in
`other.getQualifiedExtraAttributes()` map are stripped of qualifiers as
explained above.
That is why I introduced parameterised version of
`ExtendableItemHelper::getExtraAttributes()` used by `XmlReportParser`, this
way parsed dependencies have both versions of qualifiers, same as resolved
dependencies.
As I pointed out in original PR message, issue may instead be solved by
modifying `ModuleRevisionId::equals()` to use `super.extraAttributes`, provided
attributes namespaces cannot clash, of course, which will result in ignoring
qualifiers:
```Java
@Override
public boolean equals(Object obj) {
...
return other.getRevision().equals(getRevision())
&& !(other.getBranch() == null && getBranch() != null)
&& !(other.getBranch() != null &&
!other.getBranch().equals(getBranch()))
&& other.getModuleId().equals(getModuleId())
&& other.getExtraAttributes().equals(getExtraAttributes()); //<
ignoring qualifiers
}
```
At what point do you suggest we check for presense of `':'` in the names of
attributes?
---
-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...
Github user aprelev commented on the issue: https://github.com/apache/ant-ivy/pull/73 @twogee Can you point me in the right direction regarding namespace URL? And what are your thoughts on including the namespace prefix in the report? --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...
Github user aprelev commented on the issue: https://github.com/apache/ant-ivy/pull/73 Should I reopen PR? --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...
Github user aprelev commented on the issue:
https://github.com/apache/ant-ivy/pull/73
> Rather than hardcoding prefix, shouldn't we retrieve it by URL
"http://ant.apache.org/ivy/extra;?
Can you elaborate on prefix retrieving? The **extra-** prefix is [hardcoded
at](https://github.com/apache/ant-ivy/blob/5918182e0d6836d89c42260da9de4428d4cbcec0/src/java/org/apache/ivy/plugins/report/XmlReportWriter.java#L175)
`XmlReportWriter::extraToString()` regardless of attribute namespace:
```Java
sb.append(prefix).append("extra-").append(entry.getKey()).append("=\"")
.append(XMLHelper.escape(entry.getValue())).append("\"");
```
Maybe including namespace in XML report by passing *qualified* attributes
to `XmlReportWriter::extraToString()` would be a better choice then?
---
-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...
Github user aprelev commented on the issue: https://github.com/apache/ant-ivy/pull/73 > Tests have passed https://builds.apache.org/job/Ivy-GithubPR/361/ Not really, my changes were not applied in this build. Test *testDepsWithAttributesChanged* should have run under *org.apache.ivy.ant/IvyResolveTest* suite. --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy issue #73: Enable XML report parser to produce qualified extra attri...
Github user aprelev commented on the issue: https://github.com/apache/ant-ivy/pull/73 By the way, latest commits 43ddccb859b94c79350ece7520af4c991c2bb5e6 and 5918182e0d6836d89c42260da9de4428d4cbcec0 introduced [nested "javaversion" element](https://github.com/apache/ant-ivy/blob/5918182e0d6836d89c42260da9de4428d4cbcec0/build.xml#L29), which breaks Jenkins CI build; I had to remove this for both - OpenJDK 1.8.0_171, Ant 1.9.6 (July 8, 2015), Ubuntu 16.04, and - Java 1.8.0_181, Ant 1.9.6 (April 16, 2016), Windows 10.1709 in order to compile Ivy. --- - To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org For additional commands, e-mail: dev-h...@ant.apache.org
[GitHub] ant-ivy pull request #73: Enable XML report parser to produce qualified extr...
GitHub user aprelev opened a pull request:
https://github.com/apache/ant-ivy/pull/73
Enable XML report parser to produce qualified extra attributes
### Issue
`${ivy.deps.changed}` is always `true` for dependencies with extra
attributes.
### Problem
`XmlReportParser::startElement()` creates revision IDs [as
follows](https://github.com/apache/ant-ivy/blob/5918182e0d6836d89c42260da9de4428d4cbcec0/src/java/org/apache/ivy/plugins/report/XmlReportParser.java#L96):
```Java
mrid = ModuleRevisionId.newInstance(organisation, module, branch, revision,
ExtendableItemHelper.getExtraAttributes(attributes, "extra-"));
```
Here, `ExtendableItemHelper::getExtraAttributes()` method returns
*unqualified* attributes of previously resolved dependencies, which are then
compared with *qualified* attributes of currently resolved dependencies in
`ConfigurationResolveReport::checkIfChanged()` [as
follows](https://github.com/apache/ant-ivy/blob/5918182e0d6836d89c42260da9de4428d4cbcec0/src/java/org/apache/ivy/core/report/ConfigurationResolveReport.java#L101):
```Java
Set previousDepSet = new HashSet<>(
Arrays.asList(parser.getDependencyRevisionIds()));
hasChanged = !previousDepSet.equals(getModuleRevisionIds());
```
which effectively renders sets of dependecies *unequal*.
### Solutions
One solution would be to compare unqualified attributes when [testing
revision IDs for
equality](https://github.com/apache/ant-ivy/blob/5918182e0d6836d89c42260da9de4428d4cbcec0/src/java/org/apache/ivy/core/module/id/ModuleRevisionId.java#L237);
another one would be to [produce qualified
attributes](https://github.com/aprelev/ant-ivy/blob/1d508c14bbc68411b9b215f2e4e552fe20d3ae1a/src/java/org/apache/ivy/util/extendable/ExtendableItemHelper.java#L37)
when parsing XML report.
I've implemented second solution (it seemed cleaner to me), and included
unit-test to demonstrate the issue.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aprelev/ant-ivy master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/ant-ivy/pull/73.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #73
commit 1d508c14bbc68411b9b215f2e4e552fe20d3ae1a
Author: aprelev
Date: 2018-07-31T14:15:26Z
Enable XML report parser to produce qualified extra attributes
---
-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
