Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.1] Fix the build tools #43207

Merged
merged 4 commits into from Apr 5, 2024
Merged

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Apr 4, 2024

Pull Request for Issue # .

Summary of Changes

  • the JS cssversioning script needs to be aware where is the current cmd
  • it needs to respect .min files

Testing Instructions

Run php build/build.php and check the build/tmp/media folder that all the css files with a URL() (ie font awesome) end with something like ?3jhff

Download the package from https://artifacts.joomla.org/drone/joomla/joomla-cms/5.1-dev/43207/downloads/75243/

Extract the files

Check that the file media/templates/administrator/atum/css/vendor/fontawesome-free/fontawesome.css and media/templates/administrator/atum/css/vendor/fontawesome-free/fontawesome..min.css start with @charset "UTF-8";

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

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

@bembelimen @LadySolveig

@bembelimen
Copy link
Contributor

Thank you, will test it asap.

@HLeithner
Copy link
Member

I have tested this item 🔴 unsuccessfully on 79d2df0

Still the same none charset css files


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

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Apr 4, 2024

Still the same none charset css files

My test instructions are wrong, this needs a new tagged version (ie 5.1.0-RC2) with the code of this PR. This is because of this line:

system($systemGit . ' archive ' . $remote . ' | tar -x -C ' . $fullpath);
and the fact that the code of this PR is actually git versioned, in sort it's ignored as the script is getting the tools from the tagged git branch...

I have no clue how this could be tested without merging and then tagging another RC...

@HLeithner any ideas?

UPD: well, I'm forcing the replacement of the build folder and it works but maybe not the best approach...

@HLeithner
Copy link
Member

can you please revert this copy thing... if we need a tagged version then it's not a problem to test this.

@HLeithner
Copy link
Member

I tested the version before with a tagged release and it works, so please remove the copy code and I can merge it.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Apr 4, 2024

Ok,
will revert the php parts later on
done

@LadySolveig
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on 1489989

many thanks for your work so far @dgrammatiko

running php build/build.php still generates the output without charset in the tmp source code subfolder - e.g. build/tmp/1712314537.

The generated packages for a new 5.1 release are therefore still with none charset.


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

@dgrammatiko
Copy link
Contributor Author

@LadySolveig this is actually expected and it was requested from @HLeithner here: #43207 (comment)

The important part here is explained here: #43207 (comment)

Basically if you want to test this right now you need to manually edit the build.php file according to cb48ff9

In a real release, assuming the PR is already merged, the release leader creates a tag and publish it in the GitHub then everything would work as expected

@LadySolveig
Copy link
Contributor

I have tested this item ✅ successfully on 1489989

many thanks for your work so far @dgrammatiko

running php build/build.php still generates the output without charset in the tmp source code subfolder - e.g. build/tmp/1712314537.

The generated packages for a new 5.1 release are therefore still with none charset.

Thank you again for the explanation. Just tested with a new tag and all works as expected. 💚 🎉 I apologise for not reading the comments carefully enough.


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

@LadySolveig LadySolveig merged commit 97cf0fe into joomla:5.1-dev Apr 5, 2024
3 of 4 checks passed
@LadySolveig
Copy link
Contributor

Thank you so much for fixing this so quickly. @dgrammatiko

@dgrammatiko dgrammatiko deleted the 5.1-dev-phpbuild branch April 5, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants