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

Investigate if git submodules can be included #39

Open
asgrim opened this issue Sep 17, 2024 · 9 comments · May be fixed by #160
Open

Investigate if git submodules can be included #39

asgrim opened this issue Sep 17, 2024 · 9 comments · May be fixed by #160
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@asgrim
Copy link
Collaborator

asgrim commented Sep 17, 2024

At the moment, PIE downloads the ZIP archive, which doesn't include git submodules. mongodb does use submodules for the C libs to be included.

The .gitmodules file is present, however, the revision information is kept in .git which isn't included in the Zip archive.

@asgrim asgrim added the enhancement New feature or request label Sep 17, 2024
@asgrim asgrim added bug Something isn't working and removed enhancement New feature or request labels Sep 30, 2024
@asgrim asgrim added the help wanted Extra attention is needed label Nov 21, 2024
@alexandre-daubois alexandre-daubois linked a pull request Dec 23, 2024 that will close this issue
@asgrim
Copy link
Collaborator Author

asgrim commented Dec 24, 2024

I think this may now be possible with the following changes:

  1. we flag some way of forcing this extension to always be from source (i.e. add the ->setPreferSource() call in \Php\Pie\ComposerIntegration\ComposerIntegrationHandler). This could either be a special case we write into PIE itself, or probably better, add a composer.json configuration that Mongo (and other extensions with git modules) can specify. This will ask Composer to download the source with Git instead of the ZIP archive
  2. we add a step into \Php\Pie\Building\UnixBuild before the phpize to run git submodule update --init; this could either be based on the configuration mentioned, or maybe we could detect which git + existence of .gitmodule file + existence of .git path; a warning could be issued if .gitmodule exists but .git does not (indicating we cannot initialise submodules, since .git is missing).

I think add a composer.json config, something like this could work:

{
    "name": "somevendor/ourextension",
    ...
    "php-ext": {
        "has-git-submodules": true,
    }
}

What do you think folks @alcaeus / @GromNaN ?

@alexandre-daubois
Copy link
Contributor

I like the has-git-submodules option that triggers setPreferSource(). I'm having a look to implement it this way it the existing PR + add support for which git.

@GromNaN
Copy link
Member

GromNaN commented Jan 15, 2025

I wonder, as a package owner, we could generate a source-code archive ourselves, that would include the submodules we need, and add it to the release assets. In the same way that pie searches the matching assets for Windows, pie could look for a "source.zip" archive before downloading the default release archive.

I imagine this poses a problem of traceability, since it's possible to publish different code from what is in Git. But it's not worse than compiled versions for Windows. Apart from that, I see no reason not to.

@alexandre-daubois
Copy link
Contributor

Isn't it discouraging for package owners to have to generate an additional source code archive in the release assets? It may be less smooth than just publishing a new tag and that's it. I'd say a has-git-submodules would solve the problem. Moreover, if a source code archive is provided, we may need to have a way to provide the archive name so an option (or similar) would still be needed

@alcaeus
Copy link

alcaeus commented Jan 15, 2025

From our perspective, it's the easiest approach since we already provide a signed source archive.
Plus, it could be integrated into the same github action that compiles windows dlls, so package authors have an automated solution that they don't have to maintain.

@GromNaN
Copy link
Member

GromNaN commented Jan 15, 2025

From our perspective, it's the easiest approach since we already provide a signed source archive.

Example in the release 1.20.1:

  • mongodb-1.20.1.tgz
  • mongodb-1.20.1.tgz.sig

Plus, it could be integrated into the same github action that compiles windows dlls, so package authors have an automated solution that they don't have to maintain.

There is a shared Github Action that php extension developers can use? Awesome.

Moreover, if a source code archive is provided, we may need to have a way to provide the archive name so an option (or similar) would still be needed

We need a convention for the name, there is already one for the windows Zips.

@asgrim
Copy link
Collaborator Author

asgrim commented Jan 16, 2025

I like the has-git-submodules option that triggers setPreferSource(). I'm having a look to implement it this way it the existing PR + add support for which git.

I'd suggest holding off making any further changes for now, @alexandre-daubois, until consensus is achieved on the approach & we have a firm implementation plan, I don't want your efforts & time to be wasted if a different approach is taken.

From our perspective, it's the easiest approach since we already provide a signed source archive.

This could certainly be an option; a similar approach to what we have in the \Php\Pie\ComposerIntegration\OverrideWindowsUrlInstallListener, which basically changes the download URL before Composer downloads the zip.

We need a convention for the name, there is already one for the windows Zips.

Yes I agree;

I propose therefore:

  • An extension who wants to deviate from usual source code download could have some configuration/indicator that the zip file that Composer downloads should be overridden, probably in php-ext in your composer.json, maybe something like:
    •  {
           "name": "somevendor/ourextension",
           ...
           "php-ext": {
               "source-download-override": "GithubPackageReleaseAssets",
           }
       }
    • This value should probably be an enumeration with only two possible values initially, Default and GithubPackageReleaseAssets. If the source-download-override is not provided, Default would be used, and indicates the default Composer download mechanism should be used (or perhaps, a more precise naming such as ComposerDefaultDownload).
    • The GithubPackageReleaseAssets would locate URL of the appropriately named source code archive from the Github package assets, and a similar installer event listener to OverrideWindowsUrlInstallListener would be needed to change the dist URL to this.
  • The obvious answer for naming convention is to follow the same as the existing Windows zip, but since this is source, I am making an assumption that we don't need the compiler/target information, so perhaps:
    • php_{extension_name}-{tag}-src.zip, for example php_mongodb-1.20.1-src.zip
  • Note: I am not sure at the moment if the archive needs to be a ZIP or if a TGZ is acceptable; we would have to check into how Composer handles that, as it isn't something that PIE actually does.

Let me know your thoughts @alcaeus & @GromNaN - thank you!

@alexandre-daubois
Copy link
Contributor

I am not sure at the moment if the archive needs to be a ZIP or if a TGZ is acceptable

As far as I understand of this, Composer handles both formats. Pie could definitely support both 🙂

@GromNaN
Copy link
Member

GromNaN commented Jan 16, 2025

The obvious answer for naming convention is to follow the same as the existing Windows zip, but since this is source, I am making an assumption that we don't need the compiler/target information

This the easiest solution for us. But I would keep Tar format. This is the most obvious format for Unix systems and supported natively by PHP, while Zip requires the zip extension.

The format would be: php_{extension_name}-{tag}-src.tgz, for example php_mongodb-1.20.1-src.tgz

And we only have to change the name of the archive we already generate. The big advantage is that we can add the assets retroactively on versions that have already been released. I don't think we can change the composer.json file for released versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants