[4.3] Fix broken package build after merge of PR #39374 #39408

richard67 · 2022-12-13T11:38:28Z

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 [4.3] Massmail: Fix validation #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 -f and git checkout ..
Checkout the branch of this PR. You can do that with commands git fetch upstream pull/39408/head:test-pr-39408 and then git 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:

> [email protected] 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:

> [email protected] 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

dgrammatiko · 2022-12-13T12:18:18Z

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.}

obuisard · 2022-12-13T13:57:06Z

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.}

…build-after-pr-39374

richard67 · 2022-12-13T13:59:29Z

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39408.}

obuisard · 2022-12-13T13:59:51Z

To be honest, I love this PR's description and documentation. Thanks Richard @richard67, you made it easy to test.

SharkyKZ · 2022-12-13T14:00:02Z

It should go without saying but removing assets is a B/C break.

dgrammatiko · 2022-12-13T14:03:03Z

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

richard67 · 2022-12-13T14:07:58Z

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.

SharkyKZ · 2022-12-13T14:16:03Z

It's actually this PR that causes templates to error.

richard67 · 2022-12-13T14:20:29Z

It's actually this PR that causes templates to error.

So we shall keep not existing asset files being referenced in the assets.json?

dgrammatiko · 2022-12-13T14:24:16Z

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

richard67 · 2022-12-13T14:28:38Z

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

@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 · 2022-12-13T14:33:47Z

@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...

richard67 · 2022-12-13T14:45:54Z

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.

richard67 · 2022-12-13T14:46:43Z

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.}

dgrammatiko · 2022-12-13T15:46:01Z

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

joomla-cms/build/build-modules-js/versioning.es6.js

Line 55 in cf21844

const jAssetFile = await lstat(path);

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

richard67 · 2022-12-13T15:49:43Z

@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.

richard67 · 2022-12-13T16:17:14Z

Closing in favour of #39413 . Please test. Thanks all.

Remove admin-users-mail assets from com_users/joomla.asset..json

657b6de

joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.3-dev labels Dec 13, 2022

richard67 added the Release Blocker label Dec 13, 2022

Merge branch '4.3-dev' into 4.3-dev-fix-broken-versioning-at-package-…

a3b0504

…build-after-pr-39374

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 13, 2022

richard67 removed the RTC This Pull Request is Ready To Commit label Dec 13, 2022

richard67 mentioned this pull request Dec 13, 2022

[4.3] Handle missing file in updateAsset method of the versioning js when building packages #39413

Closed

4 tasks

richard67 closed this Dec 13, 2022

This was referenced Dec 14, 2022

[4.3] Massmail: Fix validation #39374

Merged

[4.3] Update deleted files list in script.php for upcoming 4.3.0-alpha2 #39401

Merged

richard67 removed the Release Blocker label Dec 16, 2022

richard67 mentioned this pull request Dec 16, 2022

[4.3] Restore js files deleted with PR's #38823 and #39374 in order to be b/c for layout overrides #39431

Merged

4 tasks

richard67 deleted the 4.3-dev-fix-broken-versioning-at-package-build-after-pr-39374 branch April 2, 2023 16:37

richard67 mentioned this pull request Jun 27, 2023

[5.0] Fixes the versioning of npm tools #41069

Closed

4 tasks

Uh oh!

[4.3] Fix broken package build after merge of PR #39374 #39408

[4.3] Fix broken package build after merge of PR #39374 #39408

Uh oh!

Conversation

richard67 commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Link to documentations

Uh oh!

dgrammatiko commented Dec 13, 2022

Uh oh!

obuisard commented Dec 13, 2022

Uh oh!

richard67 commented Dec 13, 2022

Uh oh!

obuisard commented Dec 13, 2022

Uh oh!

SharkyKZ commented Dec 13, 2022

Uh oh!

dgrammatiko commented Dec 13, 2022

Uh oh!

richard67 commented Dec 13, 2022

Uh oh!

SharkyKZ commented Dec 13, 2022

Uh oh!

richard67 commented Dec 13, 2022

Uh oh!

dgrammatiko commented Dec 13, 2022

Uh oh!

richard67 commented Dec 13, 2022

Uh oh!

dgrammatiko commented Dec 13, 2022

Uh oh!

richard67 commented Dec 13, 2022

Uh oh!

richard67 commented Dec 13, 2022

Uh oh!

dgrammatiko commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richard67 commented Dec 13, 2022

Uh oh!

richard67 commented Dec 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

richard67 commented Dec 13, 2022 •

edited

Loading

dgrammatiko commented Dec 13, 2022 •

edited

Loading