Feature ? PR-5.0-dev ? Pending

User tests: Successful: Unsuccessful:

avatar brianteeman
brianteeman
6 Jan 2023

Pull Request for Issue #39488

Summary of Changes

Stops building unusable update packages in non-zip file formats. Neither the joomla upgrade component or the joomla cli update use them and the manual upload and update process in the oomla upgrade component will not accept them (but you have to wait until you have uploaded the file first to find that out).

Testing Instructions

code review and run the build.php script

Actual result BEFORE applying this Pull Request

full and update packages created for non zip file formats

Expected result AFTER applying this Pull Request

no change in the available full packages but onyl the zip format update package is generated

PR has been made against 4.2 so that the packages for 4.3 are generated correctly

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

avatar joomla-cms-bot joomla-cms-bot - change - 6 Jan 2023
Category Repository
avatar brianteeman brianteeman - open - 6 Jan 2023
avatar brianteeman brianteeman - change - 6 Jan 2023
Status New Pending
avatar brianteeman brianteeman - change - 6 Jan 2023
Labels Added: ?
avatar richard67 richard67 - test_item - 7 Jan 2023 - Tested successfully
avatar richard67
richard67 - comment - 7 Jan 2023

I have tested this item successfully on 00f4612

I've run the build script with diverse combinations of call parameters, and all worked as expected.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39567.

avatar richard67
richard67 - comment - 7 Jan 2023

@brianteeman Could you complete the documentation check boxes in the description? On docs.joomla.org it will not need any change because as far as I know there is no documentation about the build script. With manual.joomla.org I'm not sure. Maybe @HLeithner knows if it needs some change there?

avatar brianteeman brianteeman - change - 7 Jan 2023
The description was changed
avatar brianteeman brianteeman - edited - 7 Jan 2023
avatar brianteeman
brianteeman - comment - 7 Jan 2023

I dont think it needs docs on either of those sites - its an internal task just in a public repo.

But happy write if needed

avatar brianteeman brianteeman - change - 7 Jan 2023
The description was changed
avatar brianteeman brianteeman - edited - 7 Jan 2023
avatar HLeithner
HLeithner - comment - 7 Jan 2023

Please don't remove the code, instead disable the default build step.

avatar richard67
richard67 - comment - 7 Jan 2023

Please don't remove the code, instead disable the default build step.

@HLeithner Does it really need that? If we one day can make the updater use these formats, the code for creating these packages can be easily copied from the code for the full installation packages here

joomla-cms/build/build.php

Lines 553 to 584 in 00f4612

// Create full archive packages.
if (!$excludeBzip2) {
$packageName = 'Joomla_' . $fullVersion . '-' . $packageStability . '-Full_Package.tar.bz2';
echo "Building " . $packageName . "... ";
system('tar --create --bzip2 --file ../packages/' . $packageName . ' * > /dev/null');
echo "done.\n";
$checksums[$packageName] = array();
}
if (!$excludeGzip) {
$packageName = 'Joomla_' . $fullVersion . '-' . $packageStability . '-Full_Package.tar.gz';
echo "Building " . $packageName . "... ";
system('tar --create --gzip --file ../packages/' . $packageName . ' * > /dev/null');
echo "done.\n";
$checksums[$packageName] = array();
}
if (!$excludeZip) {
$packageName = 'Joomla_' . $fullVersion . '-' . $packageStability . '-Full_Package.zip';
echo "Building " . $packageName . "... ";
system('zip -r ../packages/' . $packageName . ' * > /dev/null');
echo "done.\n";
$checksums[$packageName] = array();
}
if (!$excludeZstd) {
$packageName = 'Joomla_' . $fullVersion . '-' . $packageStability . '-Full_Package.tar.zst';
echo "Building " . $packageName . "... ";
system('tar --create --use-compress-program=zstd --file ../packages/' . $packageName . ' * > /dev/null');
echo "done.\n";
$checksums[$packageName] = array();
}
and then be modified.

It would complicate our logic unnecessarily if we want to keep that just for later use and disable ny additional logic for the call parameters.

avatar wojtekxtx
wojtekxtx - comment - 7 Jan 2023

Does it really need that?

@richard67: Yes, it does.

the code for creating these packages can be easily copied from the code for the full installation packages here

@richard67 when you copy code you:

  • make J! harder to maintain (you have to maintain 2 separate, but identical, copies.
  • have to document it fully,
  • introduce yet another PAV. Its undesirable from security standpoint.
  • introduce confusion amongst devs which copy is "the right one" and which copy to extend classes against.
  • many more things.
avatar richard67
richard67 - comment - 7 Jan 2023

@richard67 when you copy code you:

  • make J! harder to maintain (you have to maintain 2 separate, but identical, copies.
  • have to document it fully,
  • introduce yet another PAV. Its undesirable from security standpoint.
  • introduce confusion amongst devs which copy is "the right one" and which copy to extend classes against.
  • many more things.

@wojtekxtx All that does not apply to what I am talking about. Maybe you should check what’ve mean before blindly commenting just because reading „copy code“. The existing code for building the update and patch packages is similar to the code for building the full installation packages. So please stop with your silly comments I can also find in others issues and PRs and which in most cases do not have anything to do with the particular issue or PR.

avatar wojtekxtx
wojtekxtx - comment - 7 Jan 2023

@richard67 Im offending noone, I just voice my opinions. You are free to disagree with me, Im taking no grudge against you for this, as one can disagree on sth, normal thing. If you disagree than you can voice your opinions as well.

But, please, do not imply that my comments are silly; I may not have the level of knowledge insider like you have, but than again, just explain why Im wrong. Its a lot nicer this way.

avatar brianteeman
brianteeman - comment - 7 Jan 2023

Ignoring the troll

@HLeithner What is your logic for keeping something that will never be used. Usually you object to having extra things as it means more to maintain etc. Ju8st trying to understand

avatar brianteeman
brianteeman - comment - 7 Jan 2023

@wojtekxtx if you had a clue what you were talking about you would see that this PR removes code that is not used. But you wanting to keep it means it does all the things you say are bad.

avatar wojtekxtx
wojtekxtx - comment - 7 Jan 2023

@brianteeman Seriously? Dont you see that more features = more = code = harder to maintain? Its plain simple man.

@wojtekxtx if you had a clue what you were talking about you would see that this PR removes code that is not used. But you wanting to keep it means it does all the things you say are bad.

Just freaking NO. Read these comments once again.

avatar richard67
richard67 - comment - 7 Jan 2023

@brianteeman Seriously? Dont you see that more features = more = code = harder to maintain? Its plain simple man.

@wojtekxtx Where does this PR add new features and new code? It removes unused code, as already said. Your statement again proves that you comment without having checked what this PR here does.

avatar carlitorweb
carlitorweb - comment - 7 Jan 2023

I've run the build script with diverse combinations of call parameters

@richard67 can you tell me how do this? for run the test I will do here...

avatar brianteeman
brianteeman - comment - 7 Jan 2023

@richard67 they've already had two strikes

avatar richard67
richard67 - comment - 7 Jan 2023

I've run the build script with diverse combinations of call parameters

@richard67 can you tell me how do this? for run the test I will do here...

@carlitorweb It needs to have a git clone. Then you can fetch the changes from this PR into a branch and check out that branch on the command line:

git fetch upstream pull/39567/head:test-pr-39567
git checkout test-pr-39567

Then, still on the command line with current directory = the Joomla root, you can run php ./build/build.php --help to get a help about the parameters.

For making a build based on that branch, use the --remote=HEAD parameter.

E.g. php ./build/build.php --remote=HEAD --exclude-gzip --exclude-bzip2 and so on.

Important: It either needs to run on Linux or another unixoid OS, or when on Windows it needs WSL (Windows Subsystem for Linux) for running the build script because it uses unixoid system commands.

avatar carlitorweb
carlitorweb - comment - 7 Jan 2023

It needs to have a git clone. Then you can fetch the changes from this PR into a branch and check out that branch on the command line:

Yes this one already have it, thank.

It either needs to run on Linux or another unixoid OS, or when on Windows it needs WSL (Windows Subsystem for Linux) for running the build script because it uses unixoid system commands.

Copy that

avatar carlitorweb
carlitorweb - comment - 8 Jan 2023

@brianteeman @richard67 I apply the patch and ran: php ./build/build.php --remote=4.2-dev but the tar package was builded. is how should be or I not doing the things right?
build

avatar richard67
richard67 - comment - 8 Jan 2023

H@carlitorweb The tar packages in your screenshot are the full installation packages. They are still built (if not excluded by the parameter). What this PR here does is stop the update and patch packages being built in that format.

avatar carlitorweb
carlitorweb - comment - 8 Jan 2023

@richard67 I was sure I was missing something. Thank, I will use others paramaters before end the test. Thank again

avatar richard67
richard67 - comment - 8 Jan 2023

@carlitorweb Here the tests which I have made, as an inspiration for other testers.

  1. php ./build/build.php --help
    Result: The changes help texts are shown and they are correct.
Usage: php ./build/build.php [options]
	[options]:
		--remote=<remote>:		The git remote reference to build from (ex: `tags/3.8.6`, `4.0-dev`), defaults to the most recent tag for the repository
		--exclude-zip:			Exclude the generation of full, update and patch zip packages
		--exclude-gzip:			Exclude the generation of full .tar.gz packages. (Update and patch .tar.gz packages are never created.)
		--exclude-bzip2:		Exclude the generation of full .tar.bz2 packages. (Update and patch .tar.bz2 packages are never created.)
		--include-zstd:			Include  the generation of full .tar.zstd packages. (Update and patch .tar.zstd packages are never created.)
		--disable-patch-packages:	Disable the generation of patch packages
		--help:				Show this help output
  1. php ./build/build.php --remote=HEAD
    Result: In folder "./build/tmp/packages"
  • Joomla_4.2.6_to_4.2.7-dev-Development-Patch_Package.zip
  • Joomla_4.2.7-dev-Development-Full_Package.tar.bz2
  • Joomla_4.2.7-dev-Development-Full_Package.tar.gz
  • Joomla_4.2.7-dev-Development-Full_Package.zip
  • Joomla_4.2.7-dev-Development-Update_Package.zip
  • Joomla_4.2.x_to_4.2.7-dev-Development-Patch_Package.zip
  1. php ./build/build.php --remote=HEAD --exclude-gzip
    Result: In folder "./build/tmp/packages"
  • Joomla_4.2.6_to_4.2.7-dev-Development-Patch_Package.zip
  • Joomla_4.2.7-dev-Development-Full_Package.tar.bz2
  • Joomla_4.2.7-dev-Development-Full_Package.zip
  • Joomla_4.2.7-dev-Development-Update_Package.zip
  • Joomla_4.2.x_to_4.2.7-dev-Development-Patch_Package.zip
  1. php ./build/build.php --remote=HEAD --exclude-bzip2
    Result: In folder "./build/tmp/packages"
  • Joomla_4.2.6_to_4.2.7-dev-Development-Patch_Package.zip
  • Joomla_4.2.7-dev-Development-Full_Package.tar.gz
  • Joomla_4.2.7-dev-Development-Full_Package.zip
  • Joomla_4.2.7-dev-Development-Update_Package.zip
  • Joomla_4.2.x_to_4.2.7-dev-Development-Patch_Package.zip
  1. php ./build/build.php --remote=HEAD --exclude-gzip --exclude-bzip2
    Result: In folder "./build/tmp/packages"
  • Joomla_4.2.6_to_4.2.7-dev-Development-Patch_Package.zip
  • Joomla_4.2.7-dev-Development-Full_Package.zip
  • Joomla_4.2.7-dev-Development-Update_Package.zip
  • Joomla_4.2.x_to_4.2.7-dev-Development-Patch_Package.zip
  1. php ./build/build.php --remote=HEAD --exclude-gzip --exclude-bzip2 --include-zstd
    Result: In folder "./build/tmp/packages"
  • Joomla_4.2.6_to_4.2.7-dev-Development-Patch_Package.zip
  • Joomla_4.2.7-dev-Development-Full_Package.tar.zst
  • Joomla_4.2.7-dev-Development-Full_Package.zip
  • Joomla_4.2.7-dev-Development-Update_Package.zip
  • Joomla_4.2.x_to_4.2.7-dev-Development-Patch_Package.zip
  1. php ./build/build.php --remote=HEAD --disable-patch-packages
    Result: In folder "./build/tmp/packages"
  • Joomla_4.2.7-dev-Development-Full_Package.tar.bz2
  • Joomla_4.2.7-dev-Development-Full_Package.tar.gz
  • Joomla_4.2.7-dev-Development-Full_Package.zip
  • Joomla_4.2.7-dev-Development-Update_Package.zip
  1. php ./build/build.php --disable-patch-packages
    Result: In folder "./build/tmp/packages"
  • Joomla_4.3.0-alpha2-Alpha-Full_Package.tar.bz2
  • Joomla_4.3.0-alpha2-Alpha-Full_Package.tar.gz
  • Joomla_4.3.0-alpha2-Alpha-Full_Package.zip
  • Joomla_4.3.0-alpha2-Alpha-Update_Package.zip
    Result: In folder "./build/tmp"
  • checksums.txt
  • github_release.txt
avatar richard67
richard67 - comment - 8 Jan 2023

P.S.: Without the PR, tar.gz and tar.bz2 packages are also built for update and patch packages if not excluded by parameter.

avatar carlitorweb carlitorweb - test_item - 8 Jan 2023 - Tested successfully
avatar carlitorweb
carlitorweb - comment - 8 Jan 2023

@richard67 thank for the explanation

avatar carlitorweb
carlitorweb - comment - 8 Jan 2023

I have tested this item successfully on 00f4612


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39567.

avatar Quy Quy - change - 8 Jan 2023
Status Pending Ready to Commit
avatar Quy
Quy - comment - 8 Jan 2023

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39567.

avatar laoneo
laoneo - comment - 9 Jan 2023

This should go into 5.0 and not in a patch release. I know it is a fix, but we do not know what is done with these tarballs into the wild, so users will expect that it works during a major series.

avatar richard67
richard67 - comment - 9 Jan 2023

Setting the RLDQ label due to previous comment so it’s not merged by accident.

avatar wojtekxtx
wojtekxtx - comment - 9 Jan 2023

@richard67 Seriously I do not follow your way of thinking......

avatar brianteeman brianteeman - change - 9 Jan 2023
The description was changed
avatar brianteeman brianteeman - edited - 9 Jan 2023
avatar brianteeman
brianteeman - comment - 9 Jan 2023

Please don't remove the code, instead disable the default build step.

@HLeithner Could you explain the reasoning behind this. Trying to understand the logic of keeping and maintaining code that has no use.

This should go into 5.0 and not in a patch release. I know it is a fix, but we do not know what is done with these tarballs into the wild, so users will expect that it works during a major series.

I can see some semblance of logic for it not going in a patch release but not one for a minor.

As I wrote in the PR description.

PR has been made against 4.2 so that the packages for 4.3 are generated correctly

There is no use case for the tarballs. They cannot be used by the joomla update.

And for once we have actual statistics that we can rely on.

tar.gz 0.0014%
tar.bz2 0.0001%

avatar wojtekxtx
wojtekxtx - comment - 10 Jan 2023

Could you explain the reasoning behind this?

@brianteeman Sure thing. I think what @HLeithner has in mind is tht this code can be reused later-on (even if its of no use now). Its reasonabkle way of thinking.

tar.gz 0.0014%
tar.bz2 0.0001%

Where are these stats taken from @brianteeman ?

avatar roland-d
roland-d - comment - 10 Jan 2023

I am in favor of this PR but agree with @laoneo that a bugfix release is not the right release. Who knows what these users do with them, but breaking whose ever flow this is part of is not the way to go.

This needs to go into 5.0.

Where are these stats taken from @brianteeman ?

If you ask me, just take the amount of downloads divide by the total amount of downloads and times 100%. At the time of writing this is 248/(164035+248+194)*100 = 0.15%

avatar brianteeman
brianteeman - comment - 10 Jan 2023

sorry my bad math - forgot the * 100

avatar brianteeman
brianteeman - comment - 10 Jan 2023

I guess its a matter of impact.

avatar wojtekxtx
wojtekxtx - comment - 10 Jan 2023

impact.

@brianteeman dare to explain?

avatar Quy Quy - change - 15 Jan 2023
Status Ready to Commit Pending
avatar Hackwar Hackwar - change - 28 Feb 2023
Labels Added: ? PR-5.0-dev ?
Removed: ?
avatar Hackwar
Hackwar - comment - 28 Feb 2023

I do support this PR. We shouldn't build stuff we rarely use and which just wastes space and cpu cycles... However, while the impact right now is small, there is an impact and thus I also would change this to Joomla 5.0. I'm going to switch over the branch.

avatar Hackwar
Hackwar - comment - 28 Feb 2023

Scrap that, I'm not going to fix those conflicts. :-D

avatar brianteeman
brianteeman - comment - 28 Feb 2023

I still believe that the impact is non-existant as the tarball cannot be used and any downloads we have recorded are the exact thing we want to avoid - downloading something that wont work.

I do not believe that the provision of files in a specified format could ever be part of a B/C promise

avatar HLeithner
HLeithner - comment - 2 May 2023

This pull request has been automatically rebased to 5.0-dev.

avatar brianteeman brianteeman - change - 8 Sep 2023
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2023-09-08 11:49:09
Closed_By brianteeman
Labels Added: Feature
avatar brianteeman brianteeman - close - 8 Sep 2023

Add a Comment

Login with GitHub to post a comment