factor-lang: Restructure package for easier extension

[spacefrogg]

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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.

[nixos-discourse]

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]

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

[spacefrogg]

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.

[SuperSandro2000]

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]

lots of nits, but otherwise looking good

[nixos-discourse]

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

[inclyc]

Hi @jian-lin,

I don't have a strong opinion on the linter's styling in this PR. Perhaps we should move the discussion to #331085 to decide how to handle unused lambda formal arguments and see if we can reach a consensus.

[ShamrockLee]

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?

[ShamrockLee]

Some minor adjustments about arguments from attrs

[spacefrogg]

@ShamrockLee Thanks! I added the missing attributes. I must confess, it is not entirely clear to me when and with which arguments the inner function (mapping its arguments to attrs) is actually called.

[ShamrockLee]

I haven't got an example to test the buildFactorApplication build helper, but the framework seems much more complete after this refactoring. I'm looking forward to its merging!

[ShamrockLee]

It's more often named makeWrapperArgs. Making it a Bash array helps preserve command-line arguments containing spaces.

Suggested change

	appendToVar wrapperArgs --set GDK_PIXBUF_MODULE_FILE "$GDK_PIXBUF_MODULE_FILE"
	appendToVar wrapperArgs --prefix LD_LIBRARY_PATH : /run/opengl-driver/lib
	makeWrapperArgs+=(--set GDK_PIXBUF_MODULE_FILE "$GDK_PIXBUF_MODULE_FILE")
	makeWrapperArgs+=(--prefix LD_LIBRARY_PATH : /run/opengl-driver/lib)

[spacefrogg]

Your suggestion was my initial implementation. A previous reviewer suggested the (newer?) approach to use appendToVar. Looking into its definition in pkgs/stdenv/generic/setup.sh, it also uses arrays if applicable. So appendToVar should be properly preserving command-line arguments as well.

You are right, although appendToVar tries to use arrays to achieve the same goal, it does not seem to work as intended. Using compound assignment is the proper way to go.

[ShamrockLee]

I had thought that appendToVar only works on string-like Bash variables. Considering that, it might be a matter of design choices.

• If you want to enable users to specify makeWrapperArgs as an attribute even when __structuredAttrs is not enabled, and you don't plan to give that non-structured derivation a way to add command-line arguments containing spaces, appendToVar is the right way to go.
• If you want to enable only the structured derivation to specify makeWrapperArgs and allow non-structured derivation to specify command-line arguments containing spaces, we could use makeWrapperArgs+=().

As #318614 is merged, the first is the way forward. If it works, let's use appendToVar instead.

[ShamrockLee]

Although appendToVar tries to use arrays to achieve the same goal, it does not seem to work as intended.

What issues with appendToVar have you encountered? It seems to work in nix-shell:

nixpkgs_factor-lang on  HEAD (41a2dde) [$] 
❯ nix-shell --expr "(import ./. { }).hello.overrideAttrs { __structuredAttrs = true; }"
structuredAttrs is enabled
Using versionCheckHook

nixpkgs_factor-lang on  HEAD (41a2dde) [$] via ❄️  impure 
❯ echo "$__structuredAttrs"
1

nixpkgs_factor-lang on  HEAD (41a2dde) [$] via ❄️  impure 
❯ appendToVar foo hi

nixpkgs_factor-lang on  HEAD (41a2dde) [$] via ❄️  impure 
❯ declare -p foo
declare -a foo=([0]="hi")

nixpkgs_factor-lang on  HEAD (41a2dde) [$] via ❄️  impure 
❯ appendToVar foo Alice and Bob

nixpkgs_factor-lang on  HEAD (41a2dde) [$] via ❄️  impure 
❯ declare -p foo
declare -a foo=([0]="hi" [1]="Alice" [2]="and" [3]="Bob")

BTW, you may want to use _accumFlagsArray to harvest the flags collected by appendToVar.

[spacefrogg]

I fixed that using __strcuturedAttrs. Thanks.

[ShamrockLee]

Does that mean

Suggested change

	All extra factor vocabularies reside in
	`pkgs/development/compilers/factor-lang/scope.nix`.
	All extra Factor vocabularies are registered in `pkgs/development/compilers/factor-lang/scope.nix`.

Should we move the file into pkgs/top-level? How should people package a factor vocabulary?

[spacefrogg]

Yes it should likely go into pkgs/top-level, but I am not sure about the rules that apply for files going there and how the mechanics work to integrate packages from there into the global set.

I don't understand your last comment. The section preceeding the sentence you remarked explains how vocabularies should be packaged. Vocabs are usually only data in a rigid directory structure, no compilation needed. They are not libraries that are compiled and referenced from the Factor runtime environment. Could that be a source of confusion? Should I spell this out?

[ShamrockLee]

I have yet to gain prior experience with Factor and need to familiarize myself with it. When I started reviewing this PR, I searched the Internet for information. Please let me know if I need to include some of the concepts.

The article Creating a vocabulary for your first program from the Factor handbook states that a vocabulary could either sit inside the source tree of the program that uses it or stays outside the source tree. The article Working with code outside of the Factor source tree then suggests that an out-of-tree vocabulary could be injected either through the environment variable FATOR_ROOTS (with a comma-separated PATH-like structure) or by using the -roots compiler flag.

In my experience, Nix packages inject dependencies in three ways:

• Include them in a list and pass them to an attribute. It is generally buildInputs and nativeBuildInputs, but language-specific build helpers could provide their own (e.g., the dependencies and build-system of buildPythonPackage).
• Vendoring them through a single IFD. For example, buildGoModule makes a fixed-output derivation goModules containing all the downloaded Go modules listed in go.mod, and the hash of such FOD is specified through vendorHash. buildRustPackage also uses a similar strategy.
• Feed the dependency list to a command-line utility to generate Nix expressions containing all dependencies. This is what most of the *2nix projects do. It allows each dependency to be fetched as a separate FOD, but the package would come with a sizeable auto-generated code that would be hard to review manually.

mkFactorApplication seems to take the first approach, but I have yet to figure out which attribute it uses to accept the dependencies.

Could you show us an example of how to package a Factor vocabulary as a derivation in the manual? For example, the application painter depends on a vocabulary bresenham. How do we include bresenham when packaging painter?

[ShamrockLee]

Yes, it should likely go into pkgs/top-level, but I am not sure about the rules that apply for files going there and how the mechanics work to integrate packages from there into the global set.

It seems that no magic is happening there. We only need to move the file and change the path in the factor-lang-scope = line in all-packages.nix.

[spacefrogg]

Factor is not very mature when it comes to inclusion of out-of-tree vocabularies. FACTOR_ROOT and -roots only for end-user consumption. High-jacking them for distribution did not prove viable in my earlier attempts. Thus, I opted for the first approach, which constructs a new runtime including all vocabularies in a single directory tree.

I will package painter and bresenham as an example.

[spacefrogg]

@ShamrockLee I finally came around to update the PR. I added the bresenham library as a show-case and added painter in the manual as an example (It is not mature enough to be packaged). I also solved the issue of "propagated" inputs. So, vocabularies (factor libraries) that need certain libraries or binaries in the runtime can propagate this to consuming applications. Depending on a vocabulary automatically depends on the necessary libraries and binaries as well.

[spacefrogg]

@ShamrockLee Thanks for all your help and comments so far. I made the adaptations.