Skip to content

factor-lang: Restructure package for easier extension#287852

Merged
ShamrockLee merged 1 commit intoNixOS:masterfrom
spacefrogg:factor-rewrap
Apr 3, 2025
Merged

factor-lang: Restructure package for easier extension#287852
ShamrockLee merged 1 commit intoNixOS:masterfrom
spacefrogg:factor-rewrap

Conversation

@spacefrogg
Copy link
Contributor

@spacefrogg spacefrogg commented Feb 10, 2024

Description of changes

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.

@ofborg ofborg bot requested a review from vrthra February 10, 2024 21:09
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Feb 10, 2024
@nixos-discourse
Copy link

nixos-discourse commented Feb 14, 2024

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3438

SuperSandro2000
SuperSandro2000 reviewed Feb 15, 2024
View reviewed changes
@spacefrogg spacefrogg force-pushed the factor-rewrap branch 3 times, most recently from b99ea67 to 3c9a4d2 Compare February 21, 2024 16:15
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog This PR adds or changes release notes 6.topic: policy discussion Discuss policies to work in and around Nixpkgs labels Feb 21, 2024
@spacefrogg
Copy link
Contributor Author

spacefrogg commented Feb 21, 2024

I have also added a forgotten feature buildFactorApplication and added proper documentation in the Nixpkgs manual.

@ofborg ofborg bot removed the 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. label Feb 21, 2024
@spacefrogg
Copy link
Contributor Author

spacefrogg commented Feb 22, 2024

Regarding the change in CODEOWNERS. I would like to take the responsibility but, obviously, need some kind of higher-up approval. I can also revert the change again.

@spacefrogg spacefrogg force-pushed the factor-rewrap branch 2 times, most recently from 5c43520 to 7894f4d Compare February 29, 2024 10:08
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Mar 1, 2024

Regarding the change in CODEOWNERS. I would like to take the responsibility but, obviously, need some kind of higher-up approval. I can also revert the change again.

The problem with codeowners is, that it doesn't properly work if you do not have merge permissions :(

SuperSandro2000
SuperSandro2000 reviewed Mar 1, 2024
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

lots of nits, but otherwise looking good

@github-actions github-actions bot removed the 6.topic: policy discussion Discuss policies to work in and around Nixpkgs label Mar 2, 2024
@nixos-discourse
Copy link

nixos-discourse commented Mar 26, 2024

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3696

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 3, 2024
@spacefrogg spacefrogg removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 30, 2024
toastal
toastal reviewed Apr 2, 2025
View reviewed changes
toastal
toastal reviewed Apr 2, 2025
View reviewed changes
toastal
toastal reviewed Apr 2, 2025
View reviewed changes
@toastal
Copy link
Contributor

toastal commented Apr 2, 2025

A lot of really good work has been done here. It generally looks really good y’all. 🙂

@spacefrogg
Copy link
Contributor Author

spacefrogg commented Apr 2, 2025 •
edited
Loading

I'm hesitant to add your suggested changes, as that would void the current approval state of this PR. As this change is large and very few people seem to dare to review it (let alone merge it when it was approved), I would like to get it through. It is hanging in here for more than a year already.

EDIT: But thank you for taking a close look, nonetheless!

@toastal
Copy link
Contributor

toastal commented Apr 2, 2025

@spacefrogg Oh, I know that feeling all too well. …Which is why I left them as just comments

@ShamrockLee
Copy link
Contributor

ShamrockLee commented Apr 2, 2025

I'm hesitant to add your suggested changes, as that would void the current approval state of this PR.

I consider the formatting change necessary and will approve again after it is addressed.

@spacefrogg
Copy link
Contributor Author

spacefrogg commented Apr 2, 2025

Then you shall get them.

ShamrockLee
ShamrockLee reviewed Apr 2, 2025
View reviewed changes
@github-actions github-actions bot removed 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes labels Apr 3, 2025
toastal
toastal reviewed Apr 3, 2025
View reviewed changes
pkgs/development/compilers/factor-lang/adjust-paths-in-unit-tests.patch Outdated
Copy link
Contributor

@toastal toastal Apr 3, 2025

Choose a reason for hiding this comment

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

If we still have a little time for nits before review, this space probably doesn’t need to exist.

Copy link
Contributor Author

@spacefrogg spacefrogg Apr 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This line is part of the context of the original file.

EDIT: For clarification, and as a context line in a diff, it must start with a space character.

toastal
toastal reviewed Apr 3, 2025
View reviewed changes
toastal
toastal reviewed Apr 3, 2025
View reviewed changes
toastal
toastal reviewed Apr 3, 2025
View reviewed changes
@spacefrogg spacefrogg force-pushed the factor-rewrap branch 2 times, most recently from 49b6bab to 9846ddb Compare April 3, 2025 14:22
ShamrockLee
ShamrockLee approved these changes Apr 3, 2025
View reviewed changes
Copy link
Contributor

@ShamrockLee ShamrockLee left a comment

Choose a reason for hiding this comment

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

Aside from eyeball checks, I checked each .nix files with the pinned nixfmt (in the Nixpkgs development shell).

There's a CI check to ensure new files and already formatted files are formatted after applying the changes. I wonder why the abscent trailing commas were not caught.

@ShamrockLee ShamrockLee added the 12.approvals: 1 This PR was reviewed and approved by one person. label Apr 3, 2025
@spacefrogg
Copy link
Contributor Author

spacefrogg commented Apr 3, 2025

Because they are examples in Markdown files, I suppose? This code is likely not run through the formatter.

@spacefrogg
Copy link
Contributor Author

spacefrogg commented Apr 3, 2025

So, how can we get hold of somebody with commit access, now? I added this PR to the NixOS discourse several times over the past months.

@ShamrockLee
Copy link
Contributor

ShamrockLee commented Apr 3, 2025

Because they are examples in Markdown files, I suppose? This code is likely not run through the formatter.

I see.

So, how can we get hold of somebody with commit access, now? I added this PR to the NixOS discourse several times over the past months.

Let's merge.

@ShamrockLee ShamrockLee merged commit 1457882 into NixOS:master Apr 3, 2025
26 of 28 checks passed
@toastal
Copy link
Contributor

toastal commented Apr 3, 2025

Congrats everyone :)

@timor
Copy link
Member

timor commented Apr 3, 2025

Finally! Thanks everyone for all the effort. So this will make it into 25.05?

@spacefrogg
Copy link
Contributor Author

spacefrogg commented Apr 3, 2025

Hi there! :) Yeah, I believe it should!

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

Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@wolfgangwalther wolfgangwalther wolfgangwalther left review comments

@jian-lin jian-lin jian-lin left review comments

@ShamrockLee ShamrockLee ShamrockLee approved these changes

@vrthra vrthra Awaiting requested review from vrthra

@GetPsyched GetPsyched Awaiting requested review from GetPsyched

@hsjobeki hsjobeki Awaiting requested review from hsjobeki

@fricklerhandwerk fricklerhandwerk Awaiting requested review from fricklerhandwerk

+1 more reviewer

@toastal toastal toastal left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.