Repository navigation
Conversation
|
Maybe I forgot about an earlier decision (e.g. in the wake of the transition from Maven to CLI), but IMO the existence of a *-dbg.js file is not a broken input state. Maybe the capability to deal with it went lost somewhen, but I'm sure we used that in the past, especially for some bootable bundles (for which normally no dbg file is created, but restarting in debug mode required such a file in the past) and for files that our old tooling could not minify at that time (e.g. newer ES6 code). I searched the SAPUI5 codebase and only found one example in sap.ushell_abap (sap/ushell_abap/thirdparty/sap-xhrlib-esm-dbg.js, an ESM file). |
|
Thanks for the feedback. So is your comment only about the framing of this scenario as broken, or is it also supposed to be handled differently in the minify task? |
|
Mhmm, good question. My first reaction was "about framing the scenario as broken only". But reading the commit message again, it might be about the warning, too. sap.ushell_abap will see a warning in each build although they can't handle it differently (as we do not support ES modules?). |
|
I was also wondering about past cases. I believe this is not the first time we encounter this and have to fix bugs (around pre-existing source maps maybe?) because of it. UI5 CLI v4 seems to be able to deal with such cases while not logging a warning, right? |
I assume that the result is at least unexpected (both files end up being minified, plus a |
In the current internal untested version I can see that the I agree, we should consider changing the log from warning to verbose, as this can be expected.
It's non-deterministic if the input |
|
With the current architecture a new build that finds a Whether a But I think the crash shows we need something more than a fix in the minify task. Right now a build that fails halfway can leave files behind that the next build picks up as input. Either:
|
@d3xter666 would you mind explaining what you mean here and how it relates to this issue? So far, this is about receiving |
|
@matz3 , my point was that if we have If this is a real source file we need to manage it some way. This is the sentence that made me think that at some point a Maybe I have missunderstood how the |
That is what this PR addresses. I am not aware of a case where a crash of our build will end up using that state as input when building again, unless you explicitly point our build to a result of a previous build. And this is actually what happened. But it is outside of the UI5 CLI, so we can only handle and skip those cases. The current code did not detect or properly handle the case, but the task steps implementation now detects such faulty states, where the same resource is written by multiple executions of a step. This is unexpected when the steps runs the invocations in parallel. And it is unexpected by the design of the minify task because each invocation is supposed to be independent, and therefore can be run in parallel. If you have an idea on how we could better handle this in general, I'm open. But I would be against a solution that involves some heuristic to detect an already built projects as input. We should rather make our tasks robust against states where they would cause unstable or unexpected outputs, this this is one of them. I will go through the change again, and adjust the warning to also give a hint about minification excludes for cases where a |
de1c095 to
f9fb0d3
Compare
A -dbg.js next to its .js in the sources is unexpected: the minify task owns the -dbg.js path and generates it from the .js. The glob matched both as separate map-step keys. The .js key wrote its generated debug variant to the -dbg.js path, which the -dbg.js key also wrote - two concurrent keys writing one path, which the shared assertDistinctWrite guard rejected, so the build crashed. To stay robust rather than crash, filter the step keys to skip both members of such a pair: a .js whose -dbg.js sibling already exists is left as it is (that -dbg.js already serves as the debug variant), the -dbg.js is passed through, and a warning is logged. The sibling paths are derived with the shared getDebugName and getNonDebugName helpers, so the special .view.js / .controller.js suffixes are handled consistently. The warning states that sources normally must not contain a -dbg.js, but if the pairing is intentional (for example the file uses ECMAScript syntax the minify task does not support yet), the user should list both files in the "builder.minification.excludes" configuration of the project's ui5.yaml. The integration test reproduces the crash and pins the expected skip; unit tests cover the filter and the warning. Also add a TODO in the minify processor: a lone -dbg.js (no .js sibling) still reaches the processor and gets a -dbg-dbg.js debug variant, which is almost certainly unintended. Clarify whether such files should be skipped instead.
f9fb0d3 to
c5dd531
Compare
Done. It turned out that ESM files are supported already, but newer ECMAScript syntax might still be a valid reason, so I've adjusted the warning message. |
This branch has not been deployed
4 participants
A -dbg.js next to its .js in the sources is a broken input state: the minify task owns the -dbg.js path and generates it from the .js, so a pre-existing one must not be there. The glob matched both as separate map-step keys. The .js key wrote its generated debug variant to the -dbg.js path, which the -dbg.js key also wrote - two concurrent keys writing one path, which the shared assertDistinctWrite guard rejected, so the build crashed.
To stay robust rather than crash, filter the step keys to skip both members of such a pair: a .js whose -dbg.js sibling already exists is left as it is (that -dbg.js already serves as the debug variant), the -dbg.js is passed through, and a warning is logged. The sibling paths are derived with the shared getDebugName and getNonDebugName helpers, so the special .view.js / .controller.js suffixes are handled consistently.
The integration test reproduces the crash and pins the expected skip; unit tests cover the filter and the warning.
Also add a TODO in the minify processor: a lone -dbg.js (no .js sibling) still reaches the processor and gets a -dbg-dbg.js debug variant, which is almost certainly unintended. Clarify whether such files should be skipped instead.