First implementation of a release builder workflow

[fballiano]

Since in the future I hope OpenMage moves some of its code (coming from external projects, like Cm_RedisSession, Cm_Cache_Backend_Redis, Pelago_Emogrifier, Zend Framework) outside of its repo (for delegation of responsibility and easier upgrades) we need a release builder that pulls this external code using composer and places it in "old style" directories (like lib/Cm or app/code/community/Cm/RedisSession).

The release builder workflow that I'm publishing with this PR does exactly that, upon tagging a new release it downloads every composer dependancy, re-creates the oldstyle directories, cleansup, zips and updates the release tag with the newly created file.

You can see the result of a test release on my fork of OM here: https://github.com/fballiano/magento-lts/releases/tag/vprova123.1.6

This PR is strictly dependant on https://github.com/OpenMage/magento-lts/pull/2138 because they both won't work if they're not both merged.

[justinbeaty]

I like this! I'm wondering why though we need to do this:

mkdir -p app/code/community/Cm/RedisSession; cp -R vendor/colinmollenhour/magento-redis-session/app/code/local/Cm/RedisSession/* app/code/community/Cm/RedisSession/

When magento-hackathon/magento-composer-installer should be able to install this module from its modman file (although it would have gone into the app/code/local folder.)

Either way, this is really nice!

[fballiano]

This is to maintain compatibility for all the people that are not using composer nor modmad, and generating a zipfile that's exactly how it always was since the beginning of time to now :-)

[justinbeaty]

This is to maintain compatibility for all the people that are not using composer nor modmad, and generating a zipfile that's exactly how it always was since the beginning of time to now :-)

I do understand that, what I mean is that the composer install --ignore-platform-reqs should have been able to install the colinmollenhour/magento-redis-session module to app/code/local without the need to manually copy those files from vendor. We would still require a mv app/code/local/Cm app/code/community step to keep it consistent with the current structure, though.

[fballiano]

sorry 😅

I think it only does it if the "magento-root-dir" is set correctly in the composer.json (although it didn't do it on my local tests) but anyway I preferred to keep the module in the app/code/community directory (as it is now in the repo)

[justinbeaty]

sorry 😅

I think it only does it if the "magento-root-dir" is set correctly in the composer.json (although it didn't do it on my local tests) but anyway I preferred to keep the module in the app/code/community directory (as it is now in the repo)

Ah, that could be it indeed. I'm certainly not opposed to how you have it, I just thought it might be easier to have the magento-hackathon/magento-composer-installer repo do it. Especially if there were ever other Mage modules required this way, but I'm not sure there will ever be.

I downloaded the build zip file, I'll take a look through it all.

[Flyingmana]

This PR is strictly dependant on https://github.com/OpenMage/magento-lts/pull/2138 because they both won't work if they're not both merged.

why is it necessary? Not standing in a way, but I would feel more comfortable when we first have a release where we have the workflow, and only later move dependencies out when we verified the downloads are working properly.

[fballiano]

This PR is strictly dependant on #2138 because they both won't work if they're not both merged.

why is it necessary? Not standing in a way, but I would feel more comfortable when we first have a release where we have the workflow, and only later move dependencies out when we verified the downloads are working properly.

It's only necessary since the workflow is built to move the libraries downloaded via composer. If those libraries are managed "the old way" there's actually no need for this specific workflow (the way I've done it).

[sreichel]

DISCUSS: imho .... i'd not care about the "old way". Nowadays composer should be available everywhere. Not?

However ... nice work, but ...

1. why --ignore-platform-reqs?
2. why move CM from app/local to app/community scope?
3. have you tested cp command? Default deploy strategy for composer is symlink instead of copy an should be enforced in composer.json.

Maybe something like this ...

    "extra": {
        "magento-deploystrategy": "copy",
        "magento-deploystrategy-dev": "symlink",
        "magento-root-dir": "htdocs",
        "magento-force": true,
        "magento-core-package-type": "magento-source",
        "auto-append-gitignore": true
    },

[fballiano]

DISCUSS: imho .... i'd not care about the "old way". Nowadays composer should be available everywhere. Not?

I see your point and I kinda agree but as far as I understood somebody from our team prefers to maintain as much backward compatibility as possible

1. why --ignore-platform-reqs?

Probably because of some php extensions (it's been some months and I don't remember anymore) but at the end of the day it's not necessary that the build environment is actually a "run" environment, I'll check back again when possible.

2. why move CM from app/local to app/community scope?

This was mainly to have exactly the same directory structure we have now on our repo, where we have app/code/community/Cm and, for people that extract the release tgz on an existing docroot, this way the redis module doesn't get duplicated. I mean, it's not super necessary... I liked it because the built release was exactly the same of what we are releasing now.

3. have you tested cp command? Default deploy strategy for composer is symlink instead of copy an should be enforced in composer.json.

I'll try but point 2 is still, I'm not that much opinionated on that, although I prefer when modules install themselves in the "community" scope

[fballiano]

So, I've changed the action to run on php8 instead of the default 7.3, the ignore-platform-reqs is still required (it's used in other actions that we already have in out repo anyway) because otherwise "composer install" will not run without installing gd, sodium and soap, which are not required for building.

Problem is, if we build using php8 it will probably use some symfony packages that are not compatible with php7 so I'd like to know what @Flyingmana thinks about that.

Either we don't generate any realease (and people will have to run composer install), or should we generate as many releases as php versions?

[Flyingmana]

we build a release package, which should replace the current .tgz download. It should be as runnable without needing to execute composer. It should support the "upload via FTP GUI client", because thats something our users do use. (and which can become very complicated when we would deploy using symlinks)

the " ignore-platform-reqs" mostly because the used php version or available extensions doesnt matter for during the build. Why do we need to check for PDO for building a .tgz File?

In most other github actions it was also, because we wanted to run it in environments, which are not yet officially supported. Or not anymore supported. It makes things easier, and I got not aware of any possible problems.

Using a more recent PHP version should be ok. Then its good enough to support the workflow. And if they need it for specific other PHP versions, then they still can pay someone to contribute a build for this one. But dont bother making the php version depending on the build environment, its easier to run a composer config platform.php 8.0 right before and has the same effect as long as no plugins are breaking with the currently used php version.

[sreichel]

the " ignore-platform-reqs" mostly because the used php version or available extensions doesnt matter for during the build. Why do we need to check for PDO for building a .tgz File?

I agree with ignoring PDO (--ignore-platform-req=ext-* should work), but not with ignoring php version (not sure if setting/faking platform in composer is suiteable).

If we use composer dependencies, that rely on differt php version, we should/can not ignore php version requirements.

[fballiano]

The problem (not right now, but in the future when we integrate Pelago_Emogrifier in composer) is that this system can't work with a committed composer.lock since Emogrifier depends on some symfony components that change based on the php version :-(

[sreichel]

Just as a workaround add composer update?

[fballiano]

The new version is already able to generate multiple version of the package based on the php version: https://github.com/fballiano/openmage/releases/tag/v11.1.1

[sreichel]

https://github.com/OpenMage/magento-lts/blob/e14bb34272aa44f5c98cd0b7b532d792e93c7e4d/.github/workflows/release.yml#L28

--ignore-platform-reqs should be removed or replaced with --ignore-platform-req=ext-*, otherwise packages will be installed in latest version defined in composer.json without checking php requirement. Guess would break Emogrifier ...

[fballiano]

done!

[justinbeaty]

I made an updated builder script proposal here: https://github.com/fballiano/openmage/pull/3

[fballiano]

Today's code runs:

but:

• emogrifier is totally missing
• zero tests on the producted zip files were done

[fballiano]

We've a pickle with the new Emogrifier, which uses namespaces and it doesn't work with our normal autoloader:

                $emogrifier = \Pelago\Emogrifier\CssInliner::fromHtml($html)
                    ->inlineCss($cssToInline)
                    ->disableInlineStyleAttributesParsing();
                $processedHtml = $emogrifier->render();

if we include the "vendor" folder in the tgz everything would work just fine, but otherwise we should create another autoloader that would be injected during the building of the release, but then it won't be be tested ever.

I start to think that bundling a smaller version of the vendor folder, as shown in https://github.com/fballiano/openmage/pull/3 would be a good compromise.

What do people here think?

[fballiano]

Actually my last comment wasn't 100% correct, I made a small change to our default autoloader: and it now works as expected.

The problem was that the autoloader wasn't processing correctly namespaces. It shouldn't even be much slower than before I think.

[fballiano]

If you want to check the built tgz: https://github.com/fballiano/openmage/releases/tag/vprova123.3.4

[sreichel]

Not tested ... just reviewed ...

• matrix do we (still) need releases for different PHP version? If i remember right, it was only need for Emogrifier that required different versions when running php7.1/7.2.

• Move Sabberworm to /lib what is Sabberworm for?

• Move Cm_Cache_Backend_Redis to /lib This files should already be there?

• Switching to composer deploy strategy copy Should we add deploystrategy to current composer?

I was not a fan of having vendor dir in release, but when its easier .... go for it. Think its still better then copy files around.

[fballiano]

• matrix do we (still) need releases for different PHP version? If i remember right, it was only need for Emogrifier that required different versions when running php7.1/7.2.

I was thinking the same, I'm not sure if some sub-dependancies could be different between php versions... but i think we could remove it

• Move Sabberworm to /lib what is Sabberworm for?

it's a dependancy of emogrifier

• Move Cm_Cache_Backend_Redis to /lib This files should already be there?

I'll check

• Switching to composer deploy strategy copy Should we add deploystrategy to current composer?

I don't have an opinion on that 😅 if it's ok with you I modify composer.json instead of doing it in every build

I was not a fan of having vendor dir in release, but when its easier .... go for it. Think its still better then copy files around.

With the modification to the autoloader everything works and it looks more similar to the previous releases that everybody knows... so I like it. but without the patch to the autoloader it's not working sadly. So we have to decide which way we prefer. Since there were some PRs to support namespaces for autoloading... I guess it's ok to patch it ;-)

[sreichel]

I don't have an opinion on that sweat_smile if it's ok with you I modify composer.json instead of doing it in every build

I'd prefer to set deploystrategy to "symlink" for dev and "copy" for production.

Namespace support is nice :)

(Do we still need to copy files to lib then?)

[sreichel]

Can you verify #2781?

[fballiano]

I've made a few changes, maybe the most important is using only php7.3 since the generated versions (with multiple php versions) were the same.

to see a generated release: https://github.com/fballiano/openmage/releases/tag/vprova123.3.5 and this is the generated zip: https://github.com/fballiano/openmage/releases/download/vprova123.3.5/openmage-vprova123.3.5.zip

@sreichel I think yes we've have to still copy files to /lib because vendor is not provided

[sreichel]

@sreichel I think yes we've have to still copy files to /lib because vendor is not provided

vendor seems pretty small ... should we better keep it? (*)

(*) imho ... this work is for the minority of users, that dont use composer after years. If the zip/tar is some MB bigger, but works w/o any flaws i'd not spent much time to make it perfect)

[fballiano]

ok it's actually better for me to keep the vendor folder, I thought people weren't fond of having it in a zip which should be composerless.

and we don't need the patch to the autoloader in that case.

I'll do it asap

[fballiano]

the new zip generated including vendor, everything is much simpler: https://github.com/fballiano/openmage/releases/tag/vprova123.3.6 and it's only 1mb bigger than before