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.

Sign up for GitHub

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

Jump to bottom

pkgs/formats: add generator for PHP config files #311299

Merged
merged 1 commit into from
May 16, 2024
Merged

pkgs/formats: add generator for PHP config files #311299

merged 1 commit into from
May 16, 2024

Conversation

thenhnn
Copy link
Contributor

@thenhnn thenhnn commented May 13, 2024

Description of changes

Some PHP apps are configurable through evaluation of config.php file (file should contain php code that should set options on hashmap), for example Filesender or Flarum.

This PR adds generation of PHP configs through Nix DSL.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@thenhnn thenhnn requested a review from infinisil as a code owner May 13, 2024 09:02
@thenhnn thenhnn force-pushed the filesender-packaging-php-format branch from 127221f to 195cc4e Compare May 13, 2024 09:03
@thenhnn thenhnn changed the title pkgs/formats: add generator for PHP config files. pkgs/formats: add generator for PHP config files May 13, 2024
Ma27
Ma27 reviewed May 13, 2024
View reviewed changes
pkgs/pkgs-lib/formats.nix Outdated Show resolved Hide resolved
@thenhnn thenhnn force-pushed the filesender-packaging-php-format branch 3 times, most recently from 8e97a29 to 88802aa Compare May 13, 2024 14:58
@thenhnn thenhnn requested a review from Ma27 May 13, 2024 14:58
drupol
drupol reviewed May 14, 2024
View reviewed changes
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

I quickly reviewed and left a comment.

I'm wondering if we could validate the config with php -l by default and eventually format it using PSR-12 with PHP-CS-Fixer.

pkgs/pkgs-lib/formats.nix Outdated Show resolved Hide resolved
@thenhnn
Copy link
Contributor Author

thenhnn commented May 14, 2024 •
edited
Loading

@drupol: I quickly reviewed and left a comment.

I'm wondering if we could validate the config with php -l by default and eventually format it using PSR-12 with PHP-CS-Fixer.

I'm afraid that it will slow down evaluation a lot.

@thenhnn thenhnn force-pushed the filesender-packaging-php-format branch 2 times, most recently from e4ba680 to 508045d Compare May 14, 2024 08:08
drupol
drupol approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

LGTM, I'll wait for @jtojnar to give its feedback on this before merging.

@jtojnar jtojnar self-requested a review May 14, 2024 10:13
jtojnar
jtojnar requested changes May 14, 2024
View reviewed changes
nixos/doc/manual/development/settings-options.section.md Show resolved Hide resolved
pkgs/pkgs-lib/formats.nix Outdated Show resolved Hide resolved
pkgs/pkgs-lib/formats.nix Outdated Show resolved Hide resolved
pkgs/pkgs-lib/formats.nix Outdated Show resolved Hide resolved
nixos/doc/manual/development/settings-options.section.md Outdated Show resolved Hide resolved
pkgs/pkgs-lib/formats.nix Outdated Show resolved Hide resolved
pkgs/pkgs-lib/formats.nix Outdated Show resolved Hide resolved
pkgs/pkgs-lib/formats.nix Outdated Show resolved Hide resolved
pkgs/pkgs-lib/formats.nix Outdated Show resolved Hide resolved
pkgs/pkgs-lib/formats.nix Outdated Show resolved Hide resolved
@thenhnn thenhnn force-pushed the filesender-packaging-php-format branch from 508045d to 3c59da6 Compare May 15, 2024 06:29
@jtojnar
Copy link
Member

jtojnar commented May 15, 2024

I'm wondering if we could validate the config with php -l by default and eventually format it using PSR-12 with PHP-CS-Fixer.

I'm afraid that it will slow down evaluation a lot.

It would be done at build time so there should be negligible effect on evaluation time.

@thenhnn thenhnn force-pushed the filesender-packaging-php-format branch 2 times, most recently from db79bdc to 66b67aa Compare May 15, 2024 09:40
@drupol

This comment was marked as resolved.

Sign in to view
@thenhnn thenhnn force-pushed the filesender-packaging-php-format branch from 66b67aa to a828dc1 Compare May 15, 2024 09:43
@thenhnn

This comment was marked as resolved.

Sign in to view
@thenhnn thenhnn force-pushed the filesender-packaging-php-format branch from a828dc1 to 8d245d7 Compare May 15, 2024 09:46
@jtojnar
Copy link
Member

jtojnar commented May 15, 2024 via email •
edited
Loading

@thenhnn
Copy link
Contributor Author

thenhnn commented May 15, 2024

Question: Is the Nix indentation mechanism and function still necessary if you pass the resulting PHP code through PHP-CS-Fixer?
If we decide including PHP-CS-Fixer in the closure is fine, then Nix indentation mechanism is pointless. If we decide that the former is too heavy, then doing basic formatting ourselves would be preferred.

I'm a bit confused, why format auto-generated file at all?

@jtojnar
Copy link
Member

jtojnar commented May 15, 2024

To make it easier for humans to read when debugging.

@jtojnar
Copy link
Member

jtojnar commented May 15, 2024

Formatting is just nice to have, IMO, so no need to block on that.

@thenhnn
Copy link
Contributor Author

thenhnn commented May 15, 2024

Formatting is just nice to have, IMO, so no need to block on that.

Can we just omit it for now?

@thenhnn thenhnn force-pushed the filesender-packaging-php-format branch 2 times, most recently from 9fc64c6 to 450998b Compare May 15, 2024 17:32
@thenhnn
Copy link
Contributor Author

thenhnn commented May 15, 2024

@jtojnar I think it's ready to merge.

@jtojnar
Copy link
Member

jtojnar commented May 16, 2024

Can we just omit it for now?

Yes, that is what I meant.

I think it's ready to merge.

No other input from me, other than the minor preference for moving it to a separate file.

We should probably give @infinisil as a code owner a bit more time to react if they want.

@thenhnn thenhnn force-pushed the filesender-packaging-php-format branch from 450998b to 4d791bc Compare May 16, 2024 07:54
@thenhnn
Copy link
Contributor Author

thenhnn commented May 16, 2024

Can we just omit it for now?

Yes, that is what I meant.

I think it's ready to merge.

No other input from me, other than the minor preference for moving it to a separate file.

We should probably give @infinisil as a code owner a bit more time to react if they want.

I moved it to separate file, yeah, it's better :)

Got it, thank you!

@thenhnn thenhnn force-pushed the filesender-packaging-php-format branch from 4d791bc to 5e495dc Compare May 16, 2024 07:55
infinisil
infinisil requested changes May 16, 2024
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Not bad! Leaving some feedback :)

pkgs/pkgs-lib/formats/php/default.nix Show resolved Hide resolved
pkgs/pkgs-lib/formats/php/default.nix Outdated Show resolved Hide resolved
pkgs/pkgs-lib/formats/php/default.nix Outdated Show resolved Hide resolved
nixos/doc/manual/development/settings-options.section.md Outdated Show resolved Hide resolved
pkgs/pkgs-lib/formats/php/default.nix Show resolved Hide resolved
@thenhnn thenhnn force-pushed the filesender-packaging-php-format branch from 5e495dc to 7c4aceb Compare May 16, 2024 09:53
@thenhnn
Copy link
Contributor Author

thenhnn commented May 16, 2024

@jtojnar Can you approve changes? Or I missed something?

infinisil
infinisil approved these changes May 16, 2024
View reviewed changes
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Other than the above this looks fine to me!

@thenhnn thenhnn force-pushed the filesender-packaging-php-format branch from 7c4aceb to 083f211 Compare May 16, 2024 10:07
@thenhnn
Copy link
Contributor Author

thenhnn commented May 16, 2024

so, it's ready to merge, right?

@drupol drupol merged commit 2ca9ba3 into NixOS:master May 16, 2024
21 checks passed
@drupol
Copy link
Contributor

drupol commented May 16, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drupol drupol drupol approved these changes

@jtojnar jtojnar jtojnar approved these changes

@infinisil infinisil infinisil approved these changes

@Ma27 Ma27 Awaiting requested review from Ma27

Assignees
No one assigned
Labels
6.topic: nixos 8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@thenhnn @jtojnar @drupol @Ma27 @infinisil @wegank