[#39408] - [4.3] Fix broken package build after merge of PR #39374
- Closed
- 13 Dec 2022
- Medium
- Build: 4.3-dev
- # 39408
- Diff
- richard67:4.3-dev-fix-broken-versioning-at-package-build-after-pr-39374
User tests: Successful: Unsuccessful:
Pull Request for Issue # .
Summary of Changes
After PR #39374 has been merged, the package build e.g. with php ./build/build.php --remote=HEAD --exclude-gzip --exclude-bzip2 fails at the versioning step for the assets because PR #39374 has removed some JS but hasn't removed it from file build/media_source/com_users/joomla.asset.json.
This PR here fixes that.
It was partly also my fault. I should have noticed this when reviewing that PR, but well, nobody's perfect.
But when testing the PR it was not obvious because package building doesn't belong to our usual testing instructions and also not to the automated CI tests, and the package build which drone runs for building the testing packages seem not to include that versioning step because it hasn't failed for that PR. Everything was green.
Pinging @Hackwar just for information.
Testing Instructions
It requires a development environment (Git clone of this repo, composer, npm) either on Linux or on Windows with WSL to run the build.php script.
- Checkout the current 4.3-dev branch where #39374 has been merged in today.
- Run
php ./build/build.php --remote=HEAD --exclude-gzip --exclude-bzip2.
Result: See section "Actual result BEFORE applying this Pull Request" below. - Clean up everything with
git clean -d -x -fandgit checkout .. - Checkout the branch of this PR. You can do that with commands
git fetch upstream pull/39408/head:test-pr-39408and thengit checkout test-pr-39408. - Repeat step 2.
Result: See section "Expected result AFTER applying this Pull Request" below.
Actual result BEFORE applying this Pull Request
At the end of the output in the command window:
> joomla@4.0.0 versioning
> node build/build.js --versioning
[Error: ENOENT: no such file or directory, lstat '/home/richard/lamp/public_html/joomla-cms-4.3-dev/build/tmp/1670929783/media/com_users/js/admin-users-mail-es5.min.js'] {
errno: -2,
code: 'ENOENT',
syscall: 'lstat',
path: '/home/richard/lamp/public_html/joomla-cms-4.3-dev/build/tmp/1670929783/media/com_users/js/admin-users-mail-es5.min.js'
}
`npm run versioning` did not complete as expected.
Expected result AFTER applying this Pull Request
At the end of the output in the command window:
> joomla@4.0.0 versioning
> node build/build.js --versioning
Timer: Versioning finished in 160 ms
Cleaning checkout in /home/richard/lamp/public_html/joomla-cms-4.3-dev/build/tmp/1670930708.
Cleaning vendors.
Cleanup complete.
Generating optimized autoload files
Generated optimized autoload files containing 2381 classes
Cleaning Composer manifests in /home/richard/lamp/public_html/joomla-cms-4.3-dev/build/tmp/1670930708.
Cleanup complete.
Workspace built.
Create list of changed files from git repository for version 4.3.0-alpha2-dev.
Delete folders not included in packages.
Build full package files.
Building Joomla_4.3.0-alpha2-dev-Development-Full_Package.zip... done.
Build full update package.
Building Joomla_4.3.0-alpha2-dev-Development-Update_Package.zip... done.
Build of version 4.3.0-alpha2-dev complete!
Link to documentations
Please select:
-
Documentation link for docs.joomla.org:
-
No documentation changes for docs.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
joomla-cms-bot
- | Category | ⇒ | Repository NPM Change |
| Status | New | ⇒ | Pending |
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39408.
| Labels |
Added:
Release Blocker
NPM Resource Changed
PR-4.3-dev
|
||
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39408.
To be honest, I love this PR's description and documentation. Thanks Richard @richard67, you made it easy to test.
It should go without saying but removing assets is a B/C break.
It should go without saying but removing assets is a B/C break.
I will agree for media/system, media/legacy and media/vendor but anything else shouldn't ever be considered public API
It should go without saying but removing assets is a B/C break.
Even if this was true for the assets handled here, this PR just removes them from the json but not from the file system on update. That is done with my other PR #39401 . So the thumb down should go there and not here.
It's actually this PR that causes templates to error.
It's actually this PR that causes templates to error.
So we shall keep not existing asset files being referenced in the assets.json?
So we shall keep not existing asset files being referenced in the assets.json?
He's suggesting reverting the changes in the assets.json and keep the files. Although the B/C promise doesn't cover layouts, the parent PR will break any existing override. I'm ok with keeping the assets.json removing the files (this won't break with a PHP Exception any override) and adding a simple check on the versioning script
So we shall keep not existing asset files being referenced in the assets.json?
He's suggesting reverting the changes in the
assets.jsonand keep the files. Although the B/C promise doesn't cover layouts, the parent PR will break any existing override. I'm ok with keeping theassets.jsonremoving the files (this won't break with a PHP Exception any override) and adding a simple check on the versioning script
@dgrammatiko "the parent PR will break any existing override." ... does that mean we should revert #39374 ?
And regarding the "adding a simple check on the versioning script": If you can make a PR for this, I'd close this one here.
But then we should add some comment to the json with that PR which tells why we have these not existing assets in it.
And what should I do then with my PR #39401 ? Should I revert the last commit so the files are not deleted on update?
@dgrammatiko "the parent PR will break any existing override." ... does that mean we should revert #39374 ?
No, the B/C promise allows these kinds of breaks. What I just realised from @SharkyKZ 's comment is that if you manipulate the assets.json and remove an entry the WAM will throw a PHP exception. This should never be allowed!
About keeping or removing the files I guess the maintainers should decide on this. But I think @SharkyKZ unveiled something interesting about WAM and the jsons and probably this worth some revision of the B/C promise and set some restrictions so layouts never throw...
About keeping or removing the files I guess the maintainers should decide on this. But I think @SharkyKZ unveiled something interesting about WAM and the jsons and probably this worth some revision of the B/C promise and set some restrictions so layouts never throw...
@dgrammatiko I've pinged the other maintainers in the maintainers channel on Mattermost and hope someone can have a look on it and make a decision. Thanks for your support so far.
| Status | Ready to Commit | ⇒ | Pending |
Back to pending due to discussion. See details in previous comments.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39408.
And regarding the "adding a simple check on the versioning script": If you can make a PR for this, I'd close this one here.
@richard67 here are the changes for the versioning:
From
to
let jAssetFile;
try {
jAssetFile = await lstat(path);
} catch(err) {
final[directory].push(asset);
return;
}About the other questions: I'll say wait for the maintainers
@dgrammatiko Ok, if you are busy I can make the PR for the versioning js and mention you, and then close this one here. We have discussed on Mattermost, and it seems that will be what we do as first, leave them in the assets.json. For the long term it would be good to have a kind of deprecated logging for obsolete assets.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-12-13 16:17:11 |
| Closed_By | ⇒ | richard67 |
I have tested this item✅ successfully on 657b6de
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39408.