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

php: Use fetchFromGitHub instead of fetchurl. #155012

Closed
wants to merge 3 commits into from

Conversation

drupol
Copy link
Contributor

@drupol drupol commented Jan 14, 2022

Motivation for this change
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@drupol
Copy link
Contributor Author

drupol commented Jan 14, 2022

@ofborg build php74

@drupol
Copy link
Contributor Author

drupol commented Jan 14, 2022

@ofborg build php80

@drupol
Copy link
Contributor Author

drupol commented Jan 14, 2022

@ofborg build php81

@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Jan 14, 2022
Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Could you explain what is the purpose of this change. (And also in the commit message.)
  • The commit message should be prefixed by maintainers/team-list: and php: tags, respectively.
  • I believe we already discussed switching from published tarballs to fetchFromGitHub in the past but I cannot find any other reference than php74: init at 7.4.0 #73151 (comment).
  • This will also break users of generic.nix like nix-phps but we probably should not consider this a public API.

@drupol
Copy link
Contributor Author

drupol commented Jan 14, 2022

  • Could you explain what is the purpose of this change. (And also in the commit message.)

I though it would be cleaner to use fetchFromGitHub than fetching a single url from php.net.
It makes sense to me to download sources from Github.

Also, I would like to provide snapshot builds (relates to fossar/nix-phps#35), so I though this would
be a first step towards it (even if we are not there yet!)

* I believe we already discussed switching from published tarballs to `fetchFromGitHub` in the past but I cannot find any other reference than [#73151 (comment)](https://github.com/NixOS/nixpkgs/pull/73151#discussion_r347748862).

I guess I was not using Nixos at that time, I don't remember such discussion.

* This will also break users of `generic.nix` like [nix-phps](https://github.com/fossar/nix-phps) but we probably should not consider this a public API.

Yep, we can update it accordingly on our own quite easily.

@AndersonTorres
Copy link
Member

  • Could you explain what is the purpose of this change. (And also in the commit message.)

I though it would be cleaner to use fetchFromGitHub than fetching a single url from php.net. It makes sense to me to download sources from Github.

I don't see such a necessity.
First, Github is not something so special to be put on a higher priority than other possible source repositories.

Second, fetchurl is simpler and more intuitive than fetchFromGitHub and the other fetchers. It should be used always when possible, or at least you should have a strong reason to prefer other fetchers than fetchurl.

Third, and related with second, the Github sources are not necessarily exactly the same as the tarballed ones. Sometimes the upstream project runs a prepare-for-release script before generating the release tarball (something like run autoconf and automake to generate the configure script).
Just look at the various occurrences of autoreconfHook + fetchFromGitHub in Nixpkgs.

Fourth, and expanding the third, if the upstream project provides their own links instead of relying on Github automated facilities, I think we should follow them in that regard. Until proven otherwise, we should trust them.

Also, I would like to provide snapshot builds (relates to fossar/nix-phps#35), so I though this would be a first step towards it (even if we are not there yet!)

What about an overlay outside Nixpkgs?

@AndersonTorres
Copy link
Member

P.S.: this commit title message is the most misleading I have seen.
I was thinking in a huge restructuring of Nixpkgs, and it was just restricted to PHP...

@drupol
Copy link
Contributor Author

drupol commented Jan 14, 2022

Thanks for all your comments, it's clearer.

Sorry for the PR title, next time I'll prefix it correctly.

I will close this PR now.

@drupol drupol closed this Jan 14, 2022
@drupol drupol deleted the php/use-fetchFromGitHub branch January 14, 2022 15:38
@drupol
Copy link
Contributor Author

drupol commented Jan 20, 2022

Sorry to dig up an already closed topics, but wouldn't it be a good idea to switch to Github so @r-ryantm can create automatic pull-requests as soon as a new tag is pushed?

@Ma27
Copy link
Member

Ma27 commented Jan 20, 2022

How is this related to GitHub? r-ryantm IIRC uses repology for versioning information and can also update packages that don't do fetchFromGitHub.

@drupol
Copy link
Contributor Author

drupol commented Jan 20, 2022

I don't think it's possible to do since we are using a specific URL for downloading the PHP tarball.

@ryantm
Copy link
Member

ryantm commented Jan 20, 2022

r-ryantm uses various sources of new versions including GitHub releases (for derivations using fetchFromGitHub) and repology.

@drupol
Copy link
Contributor Author

drupol commented Jan 20, 2022

@ryantm PHP release tarballs are fetched here: https://github.com/drupol/nixpkgs/blob/master/pkgs/development/interpreters/php/generic.nix#L300 I doubt that it can be automatized by @r-ryantm.

@AndersonTorres
Copy link
Member

The bots should conform to Nixpkgs "api", not the reverse. It looks easier to update ryantm bot instead of butchering Nixpkgs.

Also, the bots are our helpers. They should make our lives easier, not us making their lives easier.

After all, without Nixpkgs none of those bots has a reason to exist.

@drupol
Copy link
Contributor Author

drupol commented Jan 22, 2022

I definitely in favor of using bots, I found this idea amazing !

What I just suggesting here is to use fetchFromGitHub so the bot is able to provide a new PR automatically when a new tag is published.
What I'm suggesting here is precisely to use the bot and reduce the manual mambo-jambo.

AFAIK, r-ryantm has never been involved into updating PHP in the past and using fetchFromGitHub would definitely fix that.

@AndersonTorres
Copy link
Member

I definitely in favor of using bots, I found this idea amazing !

What I just suggesting here is to use fetchFromGitHub so the bot is able to provide a new PR automatically when a new tag is published. What I'm suggesting here is precisely to use the bot and reduce the manual mambo-jambo.

Then you should do more than merely change the fetcher, because surprise, different sources do not necessarily correspond to the same output.

AFAIK, r-ryantm has never been involved into updating PHP in the past and using fetchFromGitHub would definitely fix that.

Then you at least should investigate if the source can be replaced instead of supposing it.

Further, remember that r-ryantm is just a dumb builder. If github sources need some preparation, it is your job to provide them (and provide that they will not break in future releases).

@AndersonTorres
Copy link
Member

AndersonTorres commented Feb 12, 2022

Just to be not misinterpreted here: changes are welcome and I encourage them, however remember to trim the edges and ensure that the new configuration does not destroy an otherwise working release.

@drupol
Copy link
Contributor Author

drupol commented Feb 12, 2022

Hi @AndersonTorres ,

Thanks for taking the time to explain! I'm glad to have some challenges, and my motivation is not gone! :)

@drupol drupol changed the title Use fetchFromGitHub instead of fetchurl. php: Use fetchFromGitHub instead of fetchurl. Sep 11, 2023
@drupol drupol mentioned this pull request Sep 11, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants