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

gforth: override swig, not swig3 #341230

Merged

Conversation

bcdarwin
Copy link
Member

Description of changes

Removes one of two remaining usages of the top-level swig3 expression. (The other comes from older LLDB versions.)

Also removes use of overrideDerivation in favour of preferred overrideAttrs.

Unfortunately gforth still needs a custom fork of swig 3.0.9, but if absolutely necessary we could perhaps remove the swig dependency altogether, at the cost of some C interface support (I don't exactly understand what is lost by removing support from the compiler vs removing access to a standalone Forth-compatible swig, though).

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.11 Release Notes (or backporting 23.11 and 24.05 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.

This removes one of two remaining usages of the top-level `swig3` expression.
(The other comes from older LLDB versions.)

Also removes use of overrideDerivation in favour of preferred overrideAttrs.
@bcdarwin bcdarwin mentioned this pull request Sep 11, 2024
13 tasks
@emilazy
Copy link
Member

emilazy commented Sep 11, 2024

I’m wondering if the better approach wouldn’t be to get rid of the old LLDBs (cc @RossComputerGuy), and then just move pkgs/development/tools/misc/swig/3.x.nix into the Gforth directory and specialize it to this version. Though if we think the SWIG derivation will continue being compatible with this old version going forward this could be fine too, especially since the 3.x and 4.x derivations are pretty duplicated currently.

@emilazy
Copy link
Member

emilazy commented Sep 11, 2024

Okay actually apparently the patch to fix old LLDB versions is really simple: swig/swig#2377 (comment). So let’s just do that.

@emilazy
Copy link
Member

emilazy commented Sep 11, 2024

And actually the patch was upstreamed as of LLVM 16: llvm/llvm-project@f0a25fe. So only LLDB 12–15 would need patching or dropping.

@emilazy
Copy link
Member

emilazy commented Sep 11, 2024

LLDB 12 and 13 are already broken (on aarch64-linux at least); 14–17 work with SWIG 4 with no code changes. I don’t entirely understand why given the upstream patch, but it seems like we can just remove the conditional entirely and be left with no SWIG 3 users other than Gforth.

(Sorry for the only tangentially‐related noise on this PR. If you think the override approach here is the most maintainable thing going forward, let’s do that.)

@bcdarwin
Copy link
Member Author

I’m wondering if the better approach wouldn’t be to get rid of the old LLDBs (cc @RossComputerGuy), and then just move pkgs/development/tools/misc/swig/3.x.nix into the Gforth directory and specialize it to this version. Though if we think the SWIG derivation will continue being compatible with this old version going forward this could be fine too, especially since the 3.x and 4.x derivations are pretty duplicated currently.

I don't mind the current overriding - swig seems fairly stable and this seems a bit cleaner than copying it over, and this one package does not really deserve a swig refactoring. (The weird pcre2 = pcre can just be solved by renaming the variable in swig4 if we care.)

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

Alright, let’s just merge this. The whole thing is kind of gross anyway :)

I’ll put up a PR for LLDB.

@emilazy emilazy merged commit 80aafed into NixOS:master Sep 12, 2024
28 checks passed
@bcdarwin bcdarwin deleted the gforth-remove-explicit-swig3-dependency branch September 12, 2024 16:08
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.

2 participants