@kriegfrj Like this?

This is a pretty naive implementation. Not sure what side-effects there are.
Some tests fail, because there are indeed bnd.bnd files with duplicate keys. e.g.

https://github.com/bndtools/bnd/blob/master/biz.aQute.bndlib.tests/testresources/ws/p1/bnd.bnd

used by

which now fails because

is not true anymore, because of the new "Duplicate property key" warning.

Thoughts?

@kriegfrj do you have any opinion on the above?

@kriegfrj do you have any opinion on the above?

You are too quick for me! Sorry for the delayed response.

The screenshot you showed (with the text changed as you noted) looks like a perfect implementation from a visual perspective.

Regarding possible side-effects: I feel that the process of working through and fixing the test failures will help to determine the answer to that question. If there are significant behaviours that rely on the presence of duplicate properties, they should already be covered by test cases.

One side effect that the implementation will need to handle: I recall (though admittedly my recollection may be a bit rusty) that there are some property types that are designed to be "mergeable" - ie, instead of silently ignoring subsequent property settings, it merges the setting with the inherited value. The implementation would need to be able to preserve this behaviour and only apply to non-mergeable properties. I think the test cases will already cover this.

There might be some test cases which have duplicate properties in them for no good reason. The one that you flagged above seems to be one of those. In that case, I think we can simply alter the bnd.bnd file in the test to remove the duplicate headers (as the headers seem to be dummy placeholders that aren't used in the test anyway).

Aside from any issues that the test cases highlight, I can't foresee any significant negative unintended side-effects of this change, because in the current implementation of bnd these headers would be silently ignored anyway and so don't have a material effect on the build results. The only negative effect (that I can see) is that someone's workspace that was previously showing as clean may start showing up with warnings. I would argue that this is actually a good thing for them in the long term as their "no warning" build result is actually a false negative. Morever, there probably shouldn't be too many instances of this happening, and the fix (in most cases) will be fairly easy (remove the useless property setting). If they have too many warnings to fix in one go, people can sweep the warnings under the carpet by adding a global -fixupmessage directive to ignore them.

Unless @pkriens has a concern, I am happy for you to proceed (with the caveats above).

I just remembered: mergeable properties still don't allow duplicate keys - they are implemented by adding suffixes to the base property name (eg, -buildpath:, -buildpath.debug:, -buildpath.production), in which case your simple implementation should still work. However, I think (as I noted above) this would have become apparent when working through the test cases too.

thanks a lot @kriegfrj

just remembered: mergeable properties still don't allow duplicate keys - they are implemented by adding suffixes to the base property name (eg, -buildpath:, -buildpath.debug:, -buildpath.production), in which case your simple implementation should still work. However, I think (as I noted above) this would have become apparent when working through the test cases too.

I think so too but just wanted to have some confirmation from the long time bnd people :)
There is another Decorated Instructions case with the + and ++ characters. But this should be similar to merged instructions you mentioned.

So I conclude that currently it looks like there is no reason to have real duplicated keys in .bnd files , since they are skipped anyway (last one wins). So the warning is not bad. It rather gives the user a hint where to cleanup to avoid unintended side-effects.

I will go forward and cleanup the .bnd files in the testcases. I did not want to do this in the first place, to first get feedback from you, if this is the right thing to do.

I interpret the 👍 from @pkriens as a a "go" too 😄

I will post an update here.

@kriegfrj @pkriens I fixed tests and also added a testcase.

Ready for review.

So I conclude that currently it looks like there is no reason to have real duplicated keys in .bnd files , since they are skipped anyway (last one wins).

Can I also double-check this comment? I think in my experience it was the first one that "won" for Import-Package, and because the first Import-Package was included via an -include: I didn't see it and didn't realise the problem.

I noticed that the test and the change are in separate commits.

Yes I will squash after review.

So I conclude that currently it looks like there is no reason to have real duplicated keys in .bnd files , since they are skipped anyway (last one wins).

Can I also double-check this comment? I think in my experience it was the first one that "won" for Import-Package, and because the first Import-Package was included via an -include: I didn't see it and didn't realise the problem.

Hmm as far as I can see it in e.g.

it just calls Map.put(k,v) under the hood IMO this is last one wins. But have not investigated more in this.

bnd files are java properties files. It is not illegal to have duplicate keys in a java properties file. So I think that we are in pedantic mode, then we can warn on duplicate keys. But otherwise we should not error or warn on duplicate keys.

bnd files are java properties files. It is not illegal to have duplicate keys in a java properties file.

Ok. But doesn't a .bnd have more knowledge than the plain technical properties?

So I think that we are in pedantic mode, then we can warn on duplicate keys. But otherwise we should not error or warn on duplicate keys.

Ok then I think we should find another approach which is more centered around the UI and not as deep as PropertiesParser. Because I think if it only warns if pendatic=true then it is not worth the effort. Who has pendantic enabled? I would like to be warned everytime about this because I think in "bnd-thinking" a duplicate is a problem (as it was for @kriegfrj ) but sure in terms of a .properties file it is legal.

So let me come up with a different way. I think about

• recording the "duplicateness" in the PropertiesParser
• but don't warn/error there
• instead create a marker in the UI layer

But otherwise we should not error or warn on duplicate keys.

One thing I still don't understand: Why can a warning break builds? (I would understand error does, but warning?) @bjhargrave

Why can a warning break builds?

I like builds where there is no output which means success. (maven is not good at this, but gradle is better). When new warnings show up, my clean output is lost (unless I -fixupmessages to get back to clean). :-)

I like builds where there is no output which means success.

I like that too :)

When new warnings show up, my clean output is lost (unless I -fixupmessages to get back to clean). :-)

But this means we can never add any new warnings anymore. Sure I understand it's debatable what qualifies to be worth to be warned of.

Anyway, thanks for the clarification.
I will try a different approach.

@bjhargrave wrote:

bnd files are java properties files. It is not illegal to have duplicate keys in a java properties file. So I think that we are in pedantic mode, then we can warn on duplicate keys. But otherwise we should not error or warn on duplicate keys.

I think that you're asking the wrong question. It is true that that every valid Bnd file is a valid Java properties file - it does not automatically follow that every valid Java properties file should be a valid Bnd file.

I believe that a duplicate key in a Bnd file is a problem - unless you can show me a valid use case, as far as I can see at best the duplicate will do nothing, or at worst it can cause the build to produce incorrect results. Not only can it produce incorrect results, but it can do so in a way that is non-intuitive and difficult to fix - as I noted in the initial issue, I ended up wasting a couple of hours on what turned out to be a simple fix, And I like to think of myself as a seasoned Bnd developer... I pity someone with less experience accidentally stumbling upon the same issue.

Based on this alone, I argue that duplicate keys should be considered "dirt" in your workspace. The question, in my mind, is not whether it should be considered dirt, but whether we want to be able to see the dirt or to hide it.

I like builds where there is no output which means success. (maven is not good at this, but gradle is better). When new warnings show up, my clean output is lost (unless I -fixupmessages to get back to clean). :-)

I like this too, but the question is how does one best obtain the clean output - by finding and removing the dirt, or by hiding/ignoring it?

And as @chrisrueger pointed out, if we were to follow this argument, we'd never add any new warnings because we might break existing builds. I don't see new warnings as a big issue because if you really want to go the "hide it" route, there is always the -fixupmessage workaroundthat allows you to restore the illusion of cleanliness. 😄

I think the fundamental question is: should a duplicate key be considered "dirt"? I think the answer is a clear yes (per above) and for that reason it should be a warning. I guess that we could argue about whether it meets the threshold for "pedantic", but unless you can identify a valid use case where a Bnd file has duplicate properties, I think it is best to introduce the warning.

@kriegfrj wrote:

I think the fundamental question is: should a duplicate key be considered "dirt"? I think the answer is a clear yes (per above) and for that reason it should be a warning. I guess that we could argue about whether it meets the threshold for "pedantic", but unless you can identify a valid use case where a Bnd file has duplicate properties, I think it is best to introduce the warning.

Well, that conclusion didn't last long... I've had a rethink.

Can I also double-check this comment? I think in my experience it was the first one that "won" for Import-Package, and because the first Import-Package was included via an -include: I didn't see it and didn't realise the problem.

Hmm as far as I can see it in e.g.

bnd/aQute.libg/src/aQute/lib/utf8properties/PropertiesParser.java

Line 170 in b0e9b19

properties.setProperty(key, value, provenance);

it just calls Map.put(k,v) under the hood IMO this is last one wins. But have not investigated more in this.

Hmm, perhaps the issue here is not that we have duplicate keys, but a question of whether it is last-one-wins vs first-one-wins.

I can now see a valid use case for duplicate keys if the syntax is last-one-wins (you override an inherited value). So maybe my problem was not simply that there was a duplicate, but that the last one wasn't winning. I'll investigate more - perhaps hold off on further implementation until I've had a chance to do this.

So maybe my problem was not simply that there was a duplicate, but that the last one wasn't winning.

@kriegfrj
Ok, good point. So it would be great if you could show a reproducer so we know what problem we need to solve.
I thought we are solving the problem of having duplicate keys within the same bnd.bnd file. But now that you mention inheritance, maybe mean a different problem.

On that note: Do you know the new "Effective" tab in the bnd-editor (recently introduced... you need current SNAPSHOT build)? I recommend you get that because it might already be of some help to spot the actual (effective) value of a property, and also takes inheritance into account:

Example:

• cnf/build.bnd

  • Header1: 0

• foo/bnd.bnd

  • Header1: 1
  • Header1: 2
  • Header1: 3

Here you see that my (duplicated) Header1 gets the value 3 (which is last-one wins) I was referring to.

Now assume I define Header1 only in cnf/build.bnd and not in foo/bnd.bnd

Now you see in the Effective-Tab of foo/bnd.bnd that the value is 0, coming from the inherited cnf/build.bnd.

Yes, i saw the effective tab pop up when i upgraded the other day - that would indeed have been a useful debugging aid!

Now you see in the Effective-Tab of foo/bnd.bnd that the value is 0, coming from the inherited cnf/build.bnd.

Yes, this is the problem I was having - my duplicate was from explicit include not implicit as with build.bnd, but likely the same root problem. Because it was in a different file it was not obvious.

I think this behaviour is counter-intuitive. I think it would be more useful behavior if the property setting in the including file should override the property setting in the included file, not the other way around. I think one of the reasons i got stuck on it was because i was assuming "last wins" semantics. But this would be a breaking change and would need to be handled carefully.

I think duplicate properties in the same file should still be a warning though, but it's actually a slightly different problem.

my duplicate was from explicit include not implicit as with build.bnd

You can handle this by prefixing the include file name with ~. From https://bnd.bndtools.org/instructions/include.html

# Don't overwrite any existing properties (my.prop, will not be overwritten by my.prop in no.overwrite)
my.prop = don't overwrite
-include ~no.overwrite

my duplicate was from explicit include

@kriegfrj Ok, after reading this sentence and the response @bjhargrave and re-reading [your initial ticket] (#6491 (comment)), I recognize I was completely on the wrong track.
I was totally not thinking about https://bnd.bndtools.org/instructions/include.html
I just saw the word "duplicate" and thought about "duplicate keys in the same file .bnd file

So I basically solved a different problem than you had in mind.

My question is:
How do we proceed? I am a bit confused.

You can handle this by prefixing the include file name with ~. From https://bnd.bndtools.org/instructions/include.html

# Don't overwrite any existing properties (my.prop, will not be overwritten by my.prop in no.overwrite)
my.prop = don't overwrite
-include ~no.overwrite

That's useful to know, thanks!

In my case though, the order of the two statements was reversed - the include was at the top of the file and the local definition came after. In your example it would at least be intuitive for the include to override the local definition because the include comes after the local definition.

If there is good reason for this counter-intuitive behaviour that's fine - however, I think that makes an informative warning all the more necessary. Like: Property definition ineffective due to duplicate in included file x.bnd; consider using ~x.bnd.

the order of the two statements was reversed

The order does not matter.

It is important to realize that the include is not handled by the parser. That is, it is not a normal text include. The properties parser will read all properties in one go and then the Properties object is inspected for the -include instruction.

@bjhargrave wrote:

The order does not matter.

It is important to realize that the include is not handled by the parser. That is, it is not a normal text include. The properties parser will read all properties in one go and then the Properties object is inspected for the -include instruction.

Thank you - that certainly explains Bnd's current behaviour and aligns with what I was seeing. However, I think it's still a worthwhile discussion to have as to whether this behaviour is counter-intuitive, and whether (and how) it might be improved.

I would argue that the intuitive understanding of:

-include: blah.bnd
my-prop: setting

... is that my-prop should override any setting of my-prop in file blah. However, the actual behaviour (as you noted) does not align with this intuitive understanding.

@chrisrueger wrote:

@kriegfrj Ok, after reading this sentence and the response @bjhargrave and re-reading [your initial ticket] (#6491 (comment)), I recognize I was completely on the wrong track.
I was totally not thinking about https://bnd.bndtools.org/instructions/include.html
I just saw the word "duplicate" and thought about "duplicate keys in the same file .bnd file

So I basically solved a different problem than you had in mind.

Me too! I did not realise that my problem was more complicated than merely having duplicate keys.

My question is:
How do we proceed? I am a bit confused.

I think we have identified separate (but related) problems:

1. Warning about duplicate properties.
2. Counter-intuitive inheritance for duplicated properties (-include overrides local property settings regardless of order).

I think ideally we should fix the counter-intuitive behaviour but that would definitely be a breaking change and would need to be handled carefully. In the meantime, implementing warnings for duplicate properties would probably alleviate the second problem even if it doesn't fix it.

Back to your current implementation: the above discussion affirmed that the properties are "last-wins", but your implementation will not flag the first property - only the second and subsequent. In other words, the first "loser" would not be flagged, and the "winner" would be flagged. Thinking of my use case, it meant that the property that was being ignored would not have been flagged with a warning - the warning would have appeared on the included file, which would be easier to miss.

Ideally, when the property is stored its location would also be stored (not sure if this happens already?), and then when the duplicate is found it should enter the warning at the old location, not at the current location. You could then have the warning say Property definition ignored; it is masked by definition <dfn> at loc <loc> or something similar.

Back to your current implementation: the above discussion affirmed that the properties are "last-wins", but your implementation will not flag the first property - only the second and subsequent. In other words, the first "loser" would not be flagged, and the "winner" would be flagged. Thinking of my use case, it meant that the property that was being ignored would not have been flagged with a warning - the warning would have appeared on the included file, which would be easier to miss.

Ideally, when the property is stored its location would also be stored (not sure if this happens already?), and then when the duplicate is found it should enter the warning at the old location, not at the current location. You could then have the warning say Property definition ignored; it is masked by definition <dfn> at loc <loc> or something similar.

I just had a quick look at the source and I believe that the above would be possible. For the processing of includes, we might need some code here somewhere which merges the properties from the included files, where the warning can be issued at the appropriate location:

I think we have identified separate (but related) problems:

I think so too, that we are dealing with separate problems and need to be careful about word choice, to not mix up certain aspects.

1. Warning about duplicate properties.
2. Counter-intuitive inheritance for duplicated properties (-include overrides local property settings regardless of order).

I would slightly rephrase to be more precise and not fall into the same trap I fell initially:

1. Warning about duplicate properties.
There are multiple cases of the word "duplicate":

• a) same-file-duplicate: the key appears multiple times within the same .bnd file (this is what my brain thought the whole time and what the current implementation tries to address).

• b) inheritance-duplicate or -include duplicate: the key was defined somewhere up in the inheritance or include chain and affects the current key in the following two ways:

  • b1) inheritance: last-wins: the property in the current file overwrites a key which was defined earlier (Provenance)
  • b2) -include: default (counter-intuitive) first-wins behaviour of -include: -include was used and the current key has no effect because of the first-wins behaviour.

We should:

• decide what of the above we want to tackle and how.
• define what we would like to written where (e.g. creating a warning marker vs. show an exclamation icon in the new Effective view).
• I don't think we currently track all the location information (line number) of each property and provenance

2. Counter-intuitive inheritance for duplicated properties (-include overrides local property settings regardless of order).

This is a different beast. But I see it was your initial problem in #6491
Since this is current default behaviour we should not change that - because of the high build-breaking risk (because this is actual behaviour change and would force people to restructure their .bnd files).
So I think we could only try to improve how we make the developer aware of that behaviour (see warning discussion in point 1) ).

@kriegfrj little update: I think I made some progress on the "unintuitive -include case" where your local property was overwritten by the include.

I just had a quick look at the source and I believe that the above would be possible. For the processing of includes, we might need some code here somewhere which merges the properties from the included files, where the warning can be issued at the appropriate location:

Yes, thanks for that pointer. This is the place where I am fiddling at currently.

My Example:

# bnd.bnd
-include: a.bnd
Import-Package: a.b. ('the first loser": this gets unintuitively overwritten)

# a.bnd (the included file)
Import-Package: root

The warning will appear at 'the first loser".

This is great progress, well done! That's exactly what I think we need.

I have a minor suggestion about the wording: Instead of Overwritten key: 'Import-Package' (by include a.bnd), I would put 'Import-Package' declaration overridden by -include: a.bnd (consider using -include: ~a.bnd) - that way the user gets a pointer to what is otherwise a fairly obscure feature (I'm a fairly seasoned Bnd user and @bjhargrave's post above was the first time that I'd learned of that feature!). If you're feeling really adventurous, you could implement a Quick Fix suggestion that automatically makes the necessary change. 😄 But I think that the warning with that extra detail is a 95% solution.

Thanks @kriegfrj for the feedback.
I added a testcase which demonstrates the new warning behavior for the "Counter intuitive" include behavior:

https://github.com/bndtools/bnd/pull/6493/files#diff-d95b5c42e7cf7e4aae701dfcb129881fd6aa8e16e080e1027dc826ebd34ef9f4R861

Regarding wording: I am open for suggestions. But I like to have the message some kind of categorizing prefix.
Thus at the moment I have:

[Include Override]: 'Header1' declaration is overridden by -include: a.bnd and thus ignored (consider using -include: ~a.bnd).

Note: To reduce the cases where the warning is logged to a minimum, I currently only log if:

• the -include overwrites the current value with a different value
• means: if it overwrites it with the same value, I do not log a warning

Conclusion:

This PR now contains handling for the two cases of "duplicates"

a) same-file-duplicate: the key appears multiple times within the same .bnd file
b) -include overwriting: the key was overridden by -include: a.bnd instruction

At the end we should decide, if we really want to solve a) and b) both or just b). We could splitt the PR in multiple PRs if we want, because it was mainly my / our initial misunderstanding of the problems.

Note: I will do cleanup, commit squashing at the end. Let's focus on getting the functionality right for now.

@kriegfrj @bjhargrave I have split this PR into two now. I updated the description of this PR to reflect what is left in here. So this PR remains only for the case of "Counter-intuitive -include first-wins behavior"

The new PR for the "Duplicate properties in the same .bnd file" is here:
#6501