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

wp4nix: init at 1.0.0 #169183

Merged
merged 1 commit into from
Apr 26, 2022
Merged

wp4nix: init at 1.0.0 #169183

merged 1 commit into from
Apr 26, 2022

Conversation

onny
Copy link
Contributor

@onny onny commented Apr 18, 2022

Description of changes

This PR adds the tool wp4nix. It helps packaging wordpress plugins and themes for NixOS.

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.

@mohe2015
Copy link
Contributor

I don't care in this case but generally you have to take care with authorship and copyright.

@onny
Copy link
Contributor Author

onny commented Apr 18, 2022

Sorry for that, absolutely good point. Thank you @mohe2015 for your original code over here.

@mweinelt mweinelt requested a review from dasJ April 18, 2022 15:12
Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

705a6f1 is the original commit packaging this and attribution should be given.

@onny
Copy link
Contributor Author

onny commented Apr 18, 2022

705a6f1 is the original commit packaging this and attribution should be given.

What is the best way to give attribution?

@mweinelt
Copy link
Member

mweinelt commented Apr 18, 2022

Either cherry-pick the original commit and if you made significant changes you can add yourself to Co-Authored-By: Name <you@example.com>.

Or you correct the author of the existing commit by running git commit --amend --author="Moritz Hedtke <Moritz.Hedtke@t-online.de>".

@ajs124
Copy link
Member

ajs124 commented Apr 18, 2022

As (probably?) the main author of wp4nix, I have a simple question: why would you want this in nixpkgs?

@onny
Copy link
Contributor Author

onny commented Apr 18, 2022

See here #135966
It was part of this PR. I‘m looking for a way to package wordpress themes and plugins and maybe get these packages into the NixOS repository?
I guess this tool could be useful for this purpose :)

@ajs124
Copy link
Member

ajs124 commented Apr 18, 2022

I don't think I want every wordpress plugin and theme in nixpkgs, tbh. If you look at the commits our CI pushes, the churn on those is huge.

Also, I don't really recommend anyone to run wp4nix, the program, either. If you look at what it does, it causes a lot of traffic for wordpress.org and having one instance of it running every day is more than enough.

I obviously can't and won't stop you from putting this package in nixpkgs, but as the upstream, these are my opinions.
Also, as @dasJ said, we're all surprised there is a tagged release and don't remember what that tag means.

@mohe2015
Copy link
Contributor

Will answer tomorrow as I was heavily involved.

@mweinelt
Copy link
Member

My 5 cents: If our wordpress support is mediocre and cannot be extended in a declarative fashion with themes/extensions, why are we keeping it in the first place? Then I'll assume that people are better off using a dumb container and Wordpress' auto update functionality.

@mohe2015
Copy link
Contributor

As (probably?) the main author of wp4nix, I have a simple question: why would you want this in nixpkgs?

At first I just used fetchzip with the official zip archives. Then you (#96910 (comment)) told me that they republish zips so this would've caused problems. I could've probably used fetchSvn or however it's called but it seemed like wp4nix was a better solution as it made it easier to autoupdate.

I'm now using this in #135966 to fetch the official themes/extensions so you can update them independently of the wordpress distribution.

So we're not actually telling anyone to run it to fetch the full set of themes and extensions.

@mohe2015
Copy link
Contributor

My 5 cents: If our wordpress support is mediocre and cannot be extended in a declarative fashion with themes/extensions, why are we keeping it in the first place? Then I'll assume that people are better off using a dumb container and Wordpress' auto update functionality.

I think it can be extended in a declarative fashion see #135966. I also think that this is beneficial as it allows you to only serve a read only version of the php files so they can't be overwritten that easily. Also it allows you to actually declaratively roll back your system configuration.

There are a few problems though with extensions/themes that write to places they shouldn't or do other weird stuff. But I think this is just normal packaging issues like other packages that e.g. fetch stuff while building etc.

For some (probably quite a few people who don't want to spend the effort to fix their extensions) just serving the upstream php files may be easier yes. But I think it's still beneficial to provide the possibility to do this in a more declarative way.

pkgs/development/tools/wp4nix/default.nix Outdated Show resolved Hide resolved
pkgs/development/tools/wp4nix/default.nix Outdated Show resolved Hide resolved
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.

7 participants