From 3ed56667ada453cff5598e2de58cef9814d35d90 Mon Sep 17 00:00:00 2001 From: Christoph Rueger Date: Fri, 7 Mar 2025 23:33:38 +0100 Subject: [PATCH 1/3] WIP warn Overwritten key by -include Signed-off-by: Christoph Rueger --- .../src/aQute/bnd/osgi/Processor.java | 44 +++++++++++++++++-- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/biz.aQute.bndlib/src/aQute/bnd/osgi/Processor.java b/biz.aQute.bndlib/src/aQute/bnd/osgi/Processor.java index ca23431162..283ff70ae7 100644 --- a/biz.aQute.bndlib/src/aQute/bnd/osgi/Processor.java +++ b/biz.aQute.bndlib/src/aQute/bnd/osgi/Processor.java @@ -841,19 +841,57 @@ private BiConsumer getSetterWithProvenance(File file, Properties BiConsumer set = switch (n) { case 1 -> { UTF8Properties t = (UTF8Properties) target; - yield (k, v) -> t.setProperty(k, v, file.getAbsolutePath()); + yield (k, v) -> { + String provenance = file.getAbsolutePath(); + Object prevVal = t.setProperty(k, v, provenance); + if (prevVal != null && !prevVal.equals(v)) { + warnOverwrittenByInclude(k, provenance); + } + }; } case 3 -> { UTF8Properties t = (UTF8Properties) target; UTF8Properties s = (UTF8Properties) sub; - yield (k, v) -> t.setProperty(k, v, s.getProvenance(k) - .orElse(file.getAbsolutePath())); + yield (k, v) -> { + String provenance = s.getProvenance(k) + .orElse(file.getAbsolutePath()); + Object prevVal = t.setProperty(k, v, provenance); + if (prevVal != null && !prevVal.equals(v)) { + warnOverwrittenByInclude(k, provenance); + } + }; } default -> (k, v) -> target.setProperty(k, v); }; return set; } + private void warnOverwrittenByInclude(String key, String provenance) { + SetLocation loc = warning("Overwritten key: `%s` (by include: %s) is ignored in this file.", key, + normalizeProvenance(provenance)); + try { + // try putting the warning on the "first loser" key + // whose value gets overwritten by the include + FileLine header = getHeader(key); + header.set(loc); + } catch (Exception e) { + // ignore + } + } + + private String normalizeProvenance(String provenance) { + String path = provenance; + if (path == null || path.isBlank()) { + return ""; + } + + File file = new File(path); + if (!file.isFile()) + return path; + + return normalize(file); + } + /** * This method allows a sub Processor to override recognized included files. * In general we treat files as bnd files but a sub processor can override From c0c4879e97c7ce935fd018514a87a2853d39c46a Mon Sep 17 00:00:00 2001 From: Christoph Rueger Date: Sat, 8 Mar 2025 16:27:00 +0100 Subject: [PATCH 2/3] refactor and add test testWarningOnCounterIntuitiveInclude Signed-off-by: Christoph Rueger --- .../test/test/ProcessorTest.java | 63 ++++++++++++++++++ .../src/aQute/bnd/osgi/Processor.java | 66 +++++++++++++------ 2 files changed, 110 insertions(+), 19 deletions(-) diff --git a/biz.aQute.bndlib.tests/test/test/ProcessorTest.java b/biz.aQute.bndlib.tests/test/test/ProcessorTest.java index 155f0355dc..e9cc2b4106 100644 --- a/biz.aQute.bndlib.tests/test/test/ProcessorTest.java +++ b/biz.aQute.bndlib.tests/test/test/ProcessorTest.java @@ -16,7 +16,11 @@ import java.util.List; import java.util.function.Predicate; +import org.assertj.core.api.SoftAssertions; +import org.assertj.core.api.junit.jupiter.InjectSoftAssertions; +import org.assertj.core.api.junit.jupiter.SoftAssertionsExtension; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import org.osgi.resource.Capability; import aQute.bnd.header.Attrs; @@ -37,8 +41,12 @@ import aQute.service.reporter.Reporter; import aQute.service.reporter.Reporter.SetLocation; +@ExtendWith(SoftAssertionsExtension.class) public class ProcessorTest { + @InjectSoftAssertions + SoftAssertions softly; + @Test void testMacroReferences() throws IOException { testMacroReference(""" @@ -850,4 +858,59 @@ public void testProvenance() throws IOException { } + @Test + public void testWarningOnCounterIntuitiveInclude() throws IOException { + File base = IO.getFile("generated/includeoverwrite"); + try { + base.mkdirs(); + File bnd = new File(base, "bnd.bnd"); + IO.store(""" + -include a.bnd + Header1: i will lose + """, bnd); + File sup = new File(base, "a.bnd"); + IO.store(""" + Header1: i will win + """, sup); + + try (Processor a = new Processor()) { + a.setProperties(bnd); + + softly.assertThat(a.getProperty("Header1")) + .isEqualTo("i will win"); + softly.assertThat(a.getWarnings()) + .isNotEmpty(); + softly.assertThat(a.getWarnings() + .get(0)) + .isEqualTo( + "[Include Override]: `Header1` declaration is overridden by -include: a.bnd and thus ignored (consider using -include: ~a.bnd)."); + + + } + + // now prevent the overwrite by doing what the warning says, and use + // the '~' (tilde) + + bnd = new File(base, "bnd.bnd"); + IO.store(""" + -include: ~a.bnd + Header1: now i will win, and a.bnd will not overwrite + """, bnd); + + try (Processor a = new Processor()) { + a.setProperties(bnd); + + softly.assertThat(a.getProperty("Header1")) + .isEqualTo("now i will win, and a.bnd will not overwrite"); + softly.assertThat(a.getWarnings()) + .isEmpty(); + + } + + } finally { + IO.delete(base); + } + + } + } diff --git a/biz.aQute.bndlib/src/aQute/bnd/osgi/Processor.java b/biz.aQute.bndlib/src/aQute/bnd/osgi/Processor.java index 283ff70ae7..4b2a163142 100644 --- a/biz.aQute.bndlib/src/aQute/bnd/osgi/Processor.java +++ b/biz.aQute.bndlib/src/aQute/bnd/osgi/Processor.java @@ -48,7 +48,6 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; -import java.util.function.BiConsumer; import java.util.function.BiFunction; import java.util.function.Consumer; import java.util.function.Predicate; @@ -818,7 +817,7 @@ public void doIncludeFile(File file, boolean overwrite, Properties target, Strin doIncludes(file.getParentFile(), sub); - BiConsumer set = getSetterWithProvenance(file, target, sub); + BiFunction set = getSetterWithProvenance(file, target, sub); // take care regarding overwriting properties for (Map.Entry entry : sub.entrySet()) { @@ -826,27 +825,34 @@ public void doIncludeFile(File file, boolean overwrite, Properties target, Strin String value = (String) entry.getValue(); if (overwrite || !target.containsKey(key)) { - set.accept(key, value); + SetterResult res = set.apply(key, value); + if (overwrite && res.provenance() != null && res.prevValue() != null && !res.prevValue() + .equals(value)) { + // log warning if overwrite=true and value different than + // current value + warnOverwriteByInclude(key, res.provenance()); + } } else if (extensionName != null) { String extensionKey = key + "." + extensionName; if (!target.containsKey(extensionKey)) - set.accept(extensionKey, value); + set.apply(extensionKey, value); } } } - private BiConsumer getSetterWithProvenance(File file, Properties target, Properties sub) { + private record SetterResult(Object prevValue, String provenance) {} + + private BiFunction getSetterWithProvenance(File file, Properties target, + Properties sub) { int n = target instanceof UTF8Properties ? 1 : 0; n += sub instanceof UTF8Properties ? 2 : 0; - BiConsumer set = switch (n) { + BiFunction set = switch (n) { case 1 -> { UTF8Properties t = (UTF8Properties) target; yield (k, v) -> { String provenance = file.getAbsolutePath(); - Object prevVal = t.setProperty(k, v, provenance); - if (prevVal != null && !prevVal.equals(v)) { - warnOverwrittenByInclude(k, provenance); - } + return new SetterResult(t.setProperty(k, v, provenance), provenance); + }; } case 3 -> { @@ -855,23 +861,45 @@ private BiConsumer getSetterWithProvenance(File file, Properties yield (k, v) -> { String provenance = s.getProvenance(k) .orElse(file.getAbsolutePath()); - Object prevVal = t.setProperty(k, v, provenance); - if (prevVal != null && !prevVal.equals(v)) { - warnOverwrittenByInclude(k, provenance); - } + return new SetterResult(t.setProperty(k, v, provenance), provenance); }; } - default -> (k, v) -> target.setProperty(k, v); + default -> (k, v) -> { + return new SetterResult(target.setProperty(k, v), null); + }; }; return set; } - private void warnOverwrittenByInclude(String key, String provenance) { - SetLocation loc = warning("Overwritten key: `%s` (by include: %s) is ignored in this file.", key, - normalizeProvenance(provenance)); + /** + * Logs a warning for rather "counter-intuitive" overwrite-behavior of the + * -include instruction, which can overwrite a value although the -include + * instruction is before the instruction in the current file. + *

+ * e.g. + *
+ * -include: a.bnd
+ * SomeHeader: willBeOverridden + * + *

+ * This is due to how -include works, but it was leading to confusion and + * hard to trace bugs, because users did not expect that behavior. Thus we + * now warn when this happens. + * + * @param key they overridden key + * @param provenance from where it was overridden (must not be + * null) + */ + private void warnOverwriteByInclude(String key, String provenance) { + String normalizeProvenance = normalizeProvenance(provenance); + SetLocation loc = warning( + "[Include Override]: `%s` declaration is overridden by -include: %s and thus ignored (consider using -include: ~%s).", + key, + normalizeProvenance, + normalizeProvenance); try { // try putting the warning on the "first loser" key - // whose value gets overwritten by the include + // whose value gets overridden by the include FileLine header = getHeader(key); header.set(loc); } catch (Exception e) { From 9c1519fc14325ede976ce112618e6f584fb66499 Mon Sep 17 00:00:00 2001 From: Christoph Rueger Date: Sat, 8 Mar 2025 16:55:45 +0100 Subject: [PATCH 3/3] extend test of for no warning if same value Signed-off-by: Christoph Rueger --- .../test/test/ProcessorTest.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/biz.aQute.bndlib.tests/test/test/ProcessorTest.java b/biz.aQute.bndlib.tests/test/test/ProcessorTest.java index e9cc2b4106..a4bfc80093 100644 --- a/biz.aQute.bndlib.tests/test/test/ProcessorTest.java +++ b/biz.aQute.bndlib.tests/test/test/ProcessorTest.java @@ -907,6 +907,26 @@ public void testWarningOnCounterIntuitiveInclude() throws IOException { } + // now test a case where an overwrite will happen, + // but no warning is logged, because the overwrite will overwrite + // with the same value + + bnd = new File(base, "bnd.bnd"); + IO.store(""" + -include: a.bnd + Header1: i will win + """, bnd); + + try (Processor a = new Processor()) { + a.setProperties(bnd); + + softly.assertThat(a.getProperty("Header1")) + .isEqualTo("i will win"); + softly.assertThat(a.getWarnings()) + .isEmpty(); + + } + } finally { IO.delete(base); }