knightos-scas: fix build if clang

[siraben]

Description of changes

Fix #305818

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.

[siraben]

Result of nixpkgs-review pr 305821 run on aarch64-darwin 1

2 packages built:

• knightos-scas
• knightos-z80e

[siraben]

Marked as draft because it should be better addressed by including the right header files.

[Et7f3]

Why llvm15 ? strcasecmp should be related to either standard library or darwin sdk. Maybe their is a change in llvm16 in header search path

[reckenrode]

Pinning LLVM versions adds technical debt that will have to be paid back later. It would be better to fix or mitigate the issue, so the package can continue building with newer versions of clang.

Maybe their is a change in llvm16 in header search path

Clang started rejecting implicit function declarations as errors. This was probably always a warning that was ignored, but now it’s an error.

Part of the problem was jumping to clang 16 from 11, so we had to deal with all the breaking changes at once, but clang 16 was also a very breaking release in general.

Going forward, Darwin will be updating annually after the xx.05 release of NixOS, so hopefully it will be easier to deal with breakage incrementally. I plan to open the PR for the clang 18.1 update as soon as the release is out.

[viraptor]

This would just delay the inevitable for a while. It's the upstream project that's broken here, but we have two options:

1. Set -std=c89 while compiling to likely get the previous behaviour.
2. Fix the upstream to add #include <strings.h> in expression.c

Since this applies to all systems in practice, it may be best to raise a PR upstream adding that header and then include it as a patch in nixpkgs, until a new version is released.

[siraben]

I just opened a couple of PRs that fix similar issues to this one. #305874 #305875 #305877. The problem is this is probably quite widespread among older packages. So do we want to make -std=c89 the preferred solution or a patch to be submitted to upstream? For many of these packages upstream has long since become unresponsive as well.

[Et7f3]

I think the patch based solution is better:

• other distro might see the issue search in open PR see your works
• You can use fetchpatch to get it from remote url
• Github generate patch file on the fly if you add .patch after PR url or commit url eg: https://github.com/NixOS/nixpkgs/pull/305821.patch this way you don't have to save the patch here and increasing git size repo.
• On update we don't have to touch to build instructions (the upstream didn't ask to use -std flags)

I feel that changing standard C is more trouble than adding header: some projects can use modern features but omitted an include.

exemple with a commit not yet included but github generate the patch with username=NixOs (not your): https://github.com/NixOS/nixpkgs/commit/f3b1c9618441411ccab8c0e8448bc38b41d5d750.patch if this patch is accepted this commit will be available in the real nixpkges tree and keep his url valid

[reckenrode]

I just opened a couple of PRs that fix similar issues to this one. #305874 #305875 #305877. The problem is this is probably quite widespread among older packages. So do we want to make -std=c89 the preferred solution or a patch to be submitted to upstream? For many of these packages upstream has long since become unresponsive as well.

The approach I generally followed with #234710 was to fix the broken code when possible. If there is an active upstream, I checked for a solution there first. If there was but no fix, I submitted a patch upstream and referenced that in my PRs to nixpkgs. Otherwise, I vendored the patches. I only fell back to suppressing errors with compile flags as a last resort.

[siraben]

Ok, I will update my PRs with followups that use patches.