User tests: Successful: Unsuccessful:
Pull Request for Issue #39488
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).
code review and run the build.php script
full and update packages created for non zip file formats
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
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
Category | ⇒ | Repository |
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
@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?
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
Please don't remove the code, instead disable the default build step.
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
Lines 553 to 584 in 00f4612
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.
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:
PAV
. Its undesirable from security standpoint.extend
classes against.@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.
@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.
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
@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.
@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.
@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.
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...
@richard67 they've already had two strikes
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.
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
@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?
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.
@richard67 I was sure I was missing something. Thank, I will use others paramaters before end the test. Thank again
@carlitorweb Here the tests which I have made, as an inspiration for other testers.
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
P.S.: Without the PR, tar.gz and tar.bz2 packages are also built for update and patch packages if not excluded by parameter.
@richard67 thank for the explanation
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
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.
Setting the RLDQ label due to previous comment so it’s not merged by accident.
@richard67 Seriously I do not follow your way of thinking......
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%
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 ?
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%
sorry my bad math - forgot the * 100
I guess its a matter of impact.
impact.
@brianteeman dare to explain?
Status | Ready to Commit | ⇒ | Pending |
Labels |
Added:
?
PR-5.0-dev
?
Removed: ? |
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.
Scrap that, I'm not going to fix those conflicts. :-D
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
This pull request has been automatically rebased to 5.0-dev.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-09-08 11:49:09 |
Closed_By | ⇒ | brianteeman | |
Labels |
Added:
Feature
|
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.