Signatures URLs added to any task are actually applied to all tasks

[hakanai]

Consider the case where your test configuration has commons-lang3, but main does not.
So you think you're adding the signatures only to the test task:

tasks.forbiddenApisTest {
    signaturesURLs.add(loadResource("commons-lang3-extra.txt"))
}

But then you get a build failing in forbiddenApisMain because it can't find commons-lang3's StringUtils.

This is flaky behaviour, which is why we hadn't noticed it immediately: If forbiddenApisMain runs first, it will work fine, because the configuration for forbiddenApisTest hasn't been evaluated yet. But if forbiddenApisTest runs first, it configures signaturesURLs, which turns out to be the same collection used by forbiddenApisMain. Since forbidden APIs depends on the result of compilation, and main always finishes compiling before test, this would only happen in larger projects where there are so many tasks running concurrently that it's possible for the test one to run first.

From quickly reading the code, it would look like each task uses its own collection, but it seems that the convention mapping then stomps all the references with a reference to the collection in the shared extension. So all tasks actually use the same collection object, and thus using +=/add isn't actually safe right now.

The workaround is to copy the collection first:

tasks.forbiddenApisTest {
    signaturesURLs = signaturesURLs.toMutableSet()
    signaturesURLs.add(loadResource("commons-lang3-extra.txt"))
}

Odds are, the same issue exists with all other collections, including bundledSignatures. But with bundledSignatures it's less noticeable because you're going to be using the same JDK on main and test.

Ideally, these collections would actually be SetProperty, with the initial contents of the set coming from the shared extension. But of course, that changes the public API. :(

[uschindler]

This looks indeed like a bug.

I will fix this before releasing the java 19 update.

[uschindler]

I think the easiest fix is to modify the convention mapping to make a clone:

forbidden-apis/src/main/resources/de/thetaphi/forbiddenapis/gradle/plugin-init.groovy

Line 48 in 89dd053

map(key, { extension[key] });

Should be trivial to do with some instanceof check or a simple call to clone().

[uschindler]

Hi,
it is not so easy to fix, because cloning would prevent the "add" from working, but what I figured out: If you use signaturesURLs +=, it works correctly. But if you use "add", it will of course modify the shared one. Making a clone won't help, as you would modify the clone returned by the conventions but it is not used afterwards.

Actually there's a test for the Groovy behaviour and it works (see the test gradle project). It adds "jdk-system-out" in groovy syntax only to the main source set and the test one stays unmodified. In your example if you change the code to use += it works:

forbiddenApisTest {
    signaturesURLs += loadResource("commons-lang3-extra.txt")
}

I have no idea about Kotlin, but in fact the Groovy DSL clones correctly. So it looks like affecting kotlin only. At moment I am not sure if this is a bug at all. Other tasks in Gradle behave the same. It also affects classpath. Modern plugins implemented after Kotlin now use the Property classes (but those wree used to prevent this kind of problems). Forbiddenapis is unfortuantley therefore not 100% compatible to the Kotlin DSL.

I can't change to SetProperty at the moment because of Gradle 3.x minimum dependency, so I would leave this to be fixed later in forbiddenapis v4.

[uschindler]

A final fix would be to use XxxProperty for all settings, but this would require upgrading gradle, so needs to be delayed. I will put this on the backlog.

Workaround:

• use += in Groovy DSL
• use signaturesURLs = signaturesURLs.toMutableSet() in Kotlin DSL

I will think about it a bit more, but the change to properties requires more work - and would finally remove the (outdated) convention mapping technique.

[uschindler]

I finally found a solution to fix in PR #209: I studied other old plugins and the trick is the following: Instead of returing a new instance in the convention, have some collection available as template. When the convention is executed, it takes the template, adds the extensions's data to it (addAll for Collections, source for ConfigurableFileCollections) and returns the template.

I added a Gradle-like += and a Kotlin-like with add to the sample project which gets tested. I verified that in both cases the data is kept and the extension is not modified.

I may release a bugfix version 3.4.1 this week or maybe a 3.5. I have one othe rissue to fix.