Is there a need for gradle 'clean' command

[ggleyzer]

Commenting on this issue, Cam wrote:

Remember to do ./gradlew clean before ./gradlew build

to which Markus replied:

I am curious why this advice keeps being given. There is, as far as I can currently tell, no need to run clean before build, at least for the XVM build.

My general gut feeling is that the complexity of the code in the xvm repository far exceeds an ability of the Gradle build to figure out a minimal set of actions that would be required to rebuild the distribution when something changes. However, I will try to entertain Markus's idea that it is possible and treat contradicting scenarios as bugs without polluting the issue trucker and add the build breaking scenarios as comments at this discussion.

[ggleyzer]

Here's a first one:

1. ./gradlew build

2. Add the following lines at the beginning of MethodDeclarationStatement#compileBody:

    if (method.getName().equals("createServer"))
        {
        log(errs, Severity.ERROR, Compiler.NOT_IMPLEMENTED);
        return;
        }
   

3. ./gradlew build again

The build will succeed, but will fail after ./gradlew clean

[ggleyzer]

Please just try it

[lagergren]

https://scans.gradle.com/s/36k5zvlg25l7a/failure?expanded-stacktrace=WyIwIiwiMC0xIiwiMC0xLTItMyIsIjAtMS0yIl0#1

So build scan.

We get:

[lib-xenia:compileXtc] Resolving resource dir (build): '/Users/marcus/src/xtc/pristine/lib_xenia/build/xtc/main/resources'.
[lib-xenia:compileXtc] Stamping XTC module with version: '0.4.43'
[lib-xenia] JavaExec command (launcher JavaExecLauncher): java -ea -cp /Users/marcus/src/xtc/pristine/javatools/build/libs/javatools-0.4.43.jar org.xvm.tool.Compiler -o /Users/marcus/src/xtc/pristine/lib_xenia/build/xtc/main/lib --set-version 0.4.43 -L /Users/marcus/src/xtc/pristine/lib_aggregate/build/xtc/main/lib/aggregate.xtc -L /Users/marcus/src/xtc/pristine/lib_collections/build/xtc/main/lib/collections.xtc -L /Users/marcus/src/xtc/pristine/lib_crypto/build/xtc/main/lib/crypto.xtc -L /Users/marcus/src/xtc/pristine/lib_ecstasy/build/xtc/main/lib/ecstasy.xtc -L /Users/marcus/src/xtc/pristine/lib_ecstasy/build/xtc/main/lib/javatools_turtle.xtc -L /Users/marcus/src/xtc/pristine/lib_json/build/xtc/main/lib/json.xtc -L /Users/marcus/src/xtc/pristine/lib_net/build/xtc/main/lib/net.xtc -L /Users/marcus/src/xtc/pristine/lib_web/build/xtc/main/lib/web.xtc /Users/marcus/src/xtc/pristine/lib_xenia/src/main/x/xenia.x
Error: /Users/marcus/src/xtc/pristine/lib_xenia/src/main/x/xenia.x:96 [96:5..120:6] COMPILER-NI: "{0}" is not yet implemented. ("function void () createServer(WebApp webApp, String bindAddr, UInt16 httpPort...")
[lib-xenia:compileXtc] terminated abnormally (exitValue: 255). Rethrowing exception.

So what this change appears to do is to not compile the body of any method named createServer. Xeni has such a method. There are no verbose trace-log style levels that I can follow in the javatools and I don't fully understand the evaluation order yet. Could you explain what is not up to date here, that causes this exception, that does work if I rebuild everything?

[ggleyzer]

The point of this example is to show that after a change in the compiler, build commands happily succeeds, while it should have not. Only after the clean, the build fails as expected

[lagergren]

Oh. So it's expected to fail when you add the change. This took me some time to figure out, but yes - it did look like you are breaking the compilation.

Then I understand.

So that means that if you change the compiler, i.e. javatools, we have to rebuild the XDK. This is a dependency between the compiler implementation and the compiled XDK. You were against this due to build time, but if this is the manner of the change, i.e. bootstrapping problems that really need us to rebuild the entire XDK if the javatools change, then that is something we have to start doing. Incremental builds will slowly make this faster, but it's going to take a hit. I don't think there is a way to detect this than actually trying to recompile every XDK module if the compiler internals change in such a way that their output is affected. The best thing here would be if we could warn about the compiler getting stale, but I would still have to depend on Javatools to know that, wouldn't I?

[ggleyzer]

My only point here is that Cam's comment had a merit: there are scenarios that require a clean build. That's all. I'm not asking you to change anything; I think the build dependency computation is is a decent shape. But when in doubt: clean build is a good first step

[ggleyzer]

Real life example 2 - I made a mistake subconsciously relying on the build and pushed this change after the build succeeded and all the tests passed:

[Change File API to return "immutable Byte[]" Gene Gleyzer 2/19/24, 4:09 PM ](https://github.com/xtclang/xvm/commit/94458dfe9a2540c5635dff31f89b9bd7fe590b4f)

What I didn't realize was that the jsondb library needed to be recompiled, but it was not. Hence the very next commit to fix it.

[lagergren]

This indeed seems like a dependency problem, because jsondb depends on the ecstasy module. So it should have picked that up. When it comes to bridge, I don't think that there is an explicitly registered dependency to it except for distribution contents and for its execution from inside the plugin, where it will be resolved from the dependent XDK no matter what. I guess we need to add an explict xtcModule dependency to the bridge module for the rest of the xdk, because that is magically added to the module path only when running the launchers. I would guess this would have failed the same way in a run configuration that executes the org.xvm.tool.Runner with the javatools.jar in the classpath?

The GitHub workflow tests that you can build everything from scratch, so if you had been in a branch instead of pushing stuff directly to master, you would have been notified of this, could augment the branch, and merge it when it's known to be green, which would have prevented a single breaking commit getting into master, and complicating things like automating git bisect to look for a failure and building snapshot releases on push in the future. Very much suggested.

[lagergren]

While we are trying to make everything about the build incremental, the GitHub workflow exists exactly to catch any dependency glitches still left, so that they don't get merged if the "from nothing" build state breaks. Catching any issues that breaks your intuition that gradlew build will run cleanly, is exactly what it's for. Among other things.

[ggleyzer]

I agree. Basically what you're saying is: the only way to catch any problems introduced by a change is to run a "from nothing" build. Cam's argument that stared this discussion was: clean build is the very close next second alternative to that.

[lagergren]

Yes. But we have to get away from pushing directly to master, no matter what local tests we think we have covered.

The argument is of a more general form. It goes like this (again):

There is a git workflow, that tests significantly more than you should need to or care about when just working with a simple change. It will grow. It will probably do other things. It will probably be split into checkin-test tasks so that the verification master builds cleanly. Since we want to minimise build and test time, you will get a callback from the build system 2-3 minutes after you push something to a branch, if it will preserve the buildability of master for everyone else on all platforms. But of course, if you choose to push to master, instead of having this workflow run where it's meant to, on a sub branch, to detect failures for any slightly more complex operation than the build. Then, by definition, you don't know if anything is wrong before you have broken master. Having the workflow tests run after the push to master. That is too late. Then someone else rebases and gets a broken build. This is why we have branches. It is significantly more important to keep master clean, than just checking if dependencies build cleanly.

This means you have to pick scenario A or B here.

Scenario A:

1. You will have minimal build time, which will get incrementally better. You help out by reporting any dependency issues that show up without clean, and enjoy a steadily decreased and more incremental build time. For a reproducible issue, maybe spend some time to understand how Gradle tries to calculate our dependencies, and how to fix issues with it.
2. You have an automatic job validating any change before it goes into master, that will tell you that any other party will have a working build from master, including if everything works fine and you run all sanity checks on your machine, that it will also run on our other supported platforms.

Scenario B:

1. Master will be full of "fixed broken commit that didn't build externally" and broken commits. This has been the case throughout history already, and we have to get away from there. The whole idea with a master branch is that it's at least minimally reliable. You should be able to publish at least a snapshot release for it all the time. This is not a contract we can honor if we allow changes directly to master, and this is true for any change, not just for catching a dependency bug. There must be a gatekeeper check, because master has to uphold build quality. It's not necessarily a simple "clean build worked" that makes up this check. In fact, it already tests a lot more things about the sanity, publication, consumption by third party artifacts, if it builds on Windows, etc.
2. As these validations will do more than just test if master builds clean with your changes on your host, if you refuse to branch, you will have to verify all those things manually, before you push to master, with a build (and test) time, that can never get above a certain bound. I guess you would have to virtualize Windows too, and rerun the those tests on Windows, to fulfil the contract. You never know if some file operation will break on a different OS, as we saw with the symlink issue a few weeks ago.

As one who as argued very hard about the importance of minimal build time, please help me ensure we get an incremental build, just like the build framework we are on intends. There are also cases where clean (as it just deletes directories) does not cause every dependency in the build cache to disappear, and this has also happened locally, so it's not even a perfect fail safe to make sure that there are no known dependency problems.

We have to stop working against Git semantics and ensure that our language support works. Without correct dependencies it's going to be bad. And we have to stop pushing any changes to master, if they aren't known to build cleanly on all platforms. Pushing directly to master does not scale, and it will continue on with a commit history where lots of commits aren't buildable in our main branch, which is a nightmare. It does not scale, and it's not professional.

[ggleyzer]

You switched the topic of this discussion. We are not discussing the git flow here, but the Gradle build behavior

[ggleyzer]

Also, I think we must have a way of testing any changes locally before submitting a PR.

[lagergren]

I can't reproduce what happened with your JSON-db dependency. I do the following:

build.

Go into DirectoryFileStore.x and change the code.

build without clean.

ecstasy.xtc notices that its source set has change and rebuilds.
Immediately afterwards, all modules that declare xtcModule(lib.xdk.ecstasy) as an xtcModule dependency are rebuilt, even with just a new build command ?