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

factor-lang: Restructure package for easier extension #287852

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

factor-lang: Restructure package for easier extension #287852

wants to merge 1 commit into from
+854 −34

Conversation

spacefrogg
Copy link
Contributor

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 the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Feb 10, 2024
@nixos-discourse
Copy link

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

@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 6.topic: policy discussion labels Feb 21, 2024
@spacefrogg
Copy link
Contributor Author

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

@spacefrogg
Copy link
Contributor Author

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

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/CODEOWNERS Outdated Show resolved Hide resolved
pkgs/development/compilers/factor-lang/wrapper.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/factor-lang/wrapper.nix Show resolved Hide resolved
pkgs/development/compilers/factor-lang/wrapper.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/factor-lang/wrapper.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/factor-lang/unwrapped.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/factor-lang/unwrapped.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/factor-lang/mk-factor-application.nix Outdated Show resolved Hide resolved
doc/languages-frameworks/factor.section.md Outdated Show resolved Hide resolved
doc/languages-frameworks/factor.section.md Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

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
fricklerhandwerk
fricklerhandwerk reviewed Nov 29, 2024
View reviewed changes
doc/languages-frameworks/factor.section.md Outdated
Comment on lines 172 to 191
Additional attribute that are understood by `buildFactorApplication`:
- `vocabName` is the path to the vocabulary to be deployed relative to the source root.
So, directory `foo/` from the example above could be `extra/deep/down/foo`.
This allows you to maintain Factor's vocabulary hierarchy and distribute the same source tree as a stand-alone application and as a library in the Factor development environment via the `extraVocabs` attribute.
- `binName` is the name of the resulting binary in `/bin/`.
It defaults to the last directory component in `vocabName`.
It is also added as the `meta.mainProgram` attribute to facilitate `nix run`.
- `enableUI` is `false` by default.
Set this to `true` when you ship a graphical application.
- `extraLibs` adds additional libraries as runtime dependencies.
Defaults to `[]` and is concatenated with `runtimeLibs` from the used factor-lang package.
Use `factor-minimal` to minimize the closure of runtime libraries.
- `extraPaths` adds additional binaries to the runtime PATH environment variable (without adding their libraries, as well).
Defaults to `[]` and is concatenated with `defaultBins` and `binPackages` from the used factor-lang package.
Use `factor-minimal` to minimize the closure of runtime libraries.
- `deployScriptText` is the actual deploy Factor file that is executed to deploy the application.
You can change it if you need to perform additional computation during deployment.
- `factor-lang` overrides the Factor package to use to deploy this application, which also affects the default library bindings and programs in the runtime PATH.
It defaults to `factor-lang` when `enableUI` is turned on and `factor-no-gui` when it is turned off.
Applications that use only Factor libraries without external bindings or programs may set this to `factor-minimal` or `factor-minimal-gui`.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a hard rule for me personally, because documenting all that is already great in itself, but we do have a rule for using definition lists for function arguments -- that will help migrating whatever is done to different formats programmatically.

Sorry for picking things piecemeal. I'm out again, I primarily just monitor how the redirects system is being picked up and whether it produces excess friction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I addressed your comments.

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Nov 29, 2024
@nix-owners nix-owners bot requested a review from fricklerhandwerk December 2, 2024 17:13
@spacefrogg
Copy link
Contributor Author

spacefrogg commented Dec 2, 2024 •
edited
Loading

Most build helpers would respect user-specified phases. For example, this is how buildGoModule provides its buildPhase:

https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/go/module.nix#L202

Are there solid reasons to prohibit users from specifying the buildPhase and the installPhase?

This is resolved.

@nixos-discourse
Copy link

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/4919

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 31, 2024
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 7, 2025
@spacefrogg
Copy link
Contributor Author

@ShamrockLee Polite ping, as you've requested changes (which I addressed), which might deter other reviewers with commit access to move this PR forward.

@ShamrockLee
Copy link
Contributor

@spacefrogg, thank you for being so patient, and I'm sorry for not replying for so long. I'll review this PR tomorrow afternoon (after the final exam).

@spacefrogg
Copy link
Contributor Author

@spacefrogg, thank you for being so patient, and I'm sorry for not replying for so long. I'll review this PR tomorrow afternoon (after the final exam).

Don't you worry! Most of us do this in our spare time. I wish you all the best for your exam! Thank you for looking into this.

@ShamrockLee
Copy link
Contributor

It looks like mkFactorVocabulary and the package it returns don't depend on the factor compiler.

Then why do we need another top-level package set, factorPackages, for the vocabulary packages? Would it be easier to place the Factor vocabularies inside pkgs/by-name?

@spacefrogg
Copy link
Contributor Author

spacefrogg commented Jan 9, 2025 •
edited
Loading

Right now, the design shall help to make them easier to discover. Compare it to Texlive packages, where the simplest ones also do not depend on the TeX compiler but just provide a set of source files to be used by the end-user of the Texlive distribution. This is just a consequence of languages (like Factor or TeX) that have no notion of pre-compiled libraries but compile everything from source at the very end.

Additionally, Factor (in contrast to TeX) might add such a notion (like Python or Haskell) and then we don't need to change the package organisation again.

EDIT:
Factor vocabularies (like Texlive packages) are not meant to be used by end-users but are there to compile a Factor environment with the right set of vocabularies available (just like in the Texlive case). So, it might be additionally confusing to put them under pkgs/by-name, where they are of no immediate use.

ShamrockLee
ShamrockLee reviewed Jan 9, 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.

Discussions/suggestions about some other design choices.

pkgs/development/compilers/factor-lang/mk-factor-application.nix Show resolved Hide resolved
pkgs/development/compilers/factor-lang/vocabs/bresenham/default.nix Outdated Show resolved Hide resolved
ShamrockLee
ShamrockLee reviewed Jan 9, 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.

None-blocking nits

pkgs/development/compilers/factor-lang/mk-factor-application.nix Show resolved Hide resolved
pkgs/development/compilers/factor-lang/mk-factor-application.nix Outdated Show resolved Hide resolved
pkgs/development/compilers/factor-lang/mk-factor-application.nix Outdated Show resolved Hide resolved
@ShamrockLee ShamrockLee dismissed their stale review January 10, 2025 12:44

All blocking issues are addressed.

@spacefrogg
Copy link
Contributor Author

@ShamrockLee Could I convince you to leave a positive review? :)

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

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

@ShamrockLee ShamrockLee ShamrockLee left review comments

@wolfgangwalther wolfgangwalther wolfgangwalther left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@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

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@spacefrogg @nixos-discourse @SuperSandro2000 @Aleksanaa @inclyc @jian-lin @ShamrockLee @fricklerhandwerk @wolfgangwalther @wegank