Skip to content

nesfab 1.6 (new formula) #211113

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

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

Conversation

vtbassmatt
Copy link

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Adds NESFab. NESFab is a programming language/compiler for creating NES games. It qualifies for inclusion in Homebrew because it's maintained (v1.6 came out within the last week), is known (689 GitHub stars), is used (there has been recent GitHub and Discord activity related to macOS builds), and has a homepage.

@github-actions github-actions bot added new formula PR adds a new formula to Homebrew/homebrew-core boost Boost use is a significant feature of the PR or issue labels Mar 16, 2025
Copy link
Contributor

Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

@vtbassmatt vtbassmatt force-pushed the add-nesfab branch 10 times, most recently from c51f52b to 20a8846 Compare March 16, 2025 23:41
@vtbassmatt
Copy link
Author

vtbassmatt commented Mar 17, 2025

Hi Homebrew maintainers. I'm feeling stuck after multiple rounds of searching, editing, and resubmitting. I feel like I'm missing something simple.

This project uses a C++20 feature (std::lexicographical_compare_three_way) which Clang apparently didn't fully support until libc++ 17. I searched discussions and found https://github.com/orgs/Homebrew/discussions/5977 which led to #188332, which I aped in my formula:

# ...
  on_macos do
    depends_on "llvm" => :build if DevelopmentTools.clang_build_version <= 1599
  end
# ...
  fails_with :clang do
    build 1599
    cause "Missing std::lexicographical_compare_three_way"
  end

However, the build still fails on macOS 13. (It builds on macOS 15 because I happened to get a successful run there. The PR has always failed before trying macOS 14, so I'm not confident whether it succeeds or fails there.)

I see two paths forward:

  • Correct my use of depends_on / fails_with to ensure we get a version of libc++ that works; I need someone with more Clang expertise to point me in the right direction here.
  • Remove macOS 13 (and possibly 14?) from the set of supported platforms. Upstream wouldn't build locally (by default) on that/those platforms, so this might also be acceptable.

Thanks for any help you can offer!

@botantony
Copy link
Contributor

@vtbassmatt I think this error happens because it still uses system Clang. Set CXX to the full path to Homebrew's LLVM Clang if clang_build_version is less that 1600

@vtbassmatt
Copy link
Author

@vtbassmatt I think this error happens because it still uses system Clang. Set CXX to the full path to Homebrew's LLVM Clang if clang_build_version is less that 1600

Thank you! I see now that the PR I copied from was using CXX=#{ENV.cxx} and I was not. I've kicked another build to see if this does it.

@SMillerDev
Copy link
Member

  Failed to execute /opt/homebrew/opt/llvm/bin/clang++ -w -Os -std=c++20 -pthread -Wno-unused-parameter -Wno-unknown-warning-option -Wno-narrowing -Wno-missing-field-initializers -Wno-missing-braces -Wno-unused-command-line-argument -fmax-errors=3 -ftemplate-depth=100 -pipe -Isrc -DVERSION="1.6" -DGIT_COMMIT="da18a43d-homebrew" -DNDEBUG -Wno-unused-variable -c -o obj/token.o src/token.cpp -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk --sysroot=/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk -isystem/opt/homebrew/include -isystem/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk/System/Library/Frameworks/OpenGL.framework/Versions/Current/Headers -I/opt/homebrew/opt/icu4c@77/include -I/opt/homebrew/opt/llvm@16/include -fdebug-prefix-map=/private/tmp/nesfab-20250316-8709-5r20tf/nesfab-da18a43dc7b941cc4c56c949303aa37633fdc1b0=.
  
  Xcode and/or the CLT appear to be misconfigured. Try one or both of the following:
    xcodebuild -license
    sudo xcode-select --switch /path/to/Xcode.app

@vtbassmatt
Copy link
Author

  Failed to execute /opt/homebrew/opt/llvm/bin/clang++ -w -Os -std=c++20 -pthread -Wno-unused-parameter -Wno-unknown-warning-option -Wno-narrowing -Wno-missing-field-initializers -Wno-missing-braces -Wno-unused-command-line-argument -fmax-errors=3 -ftemplate-depth=100 -pipe -Isrc -DVERSION="1.6" -DGIT_COMMIT="da18a43d-homebrew" -DNDEBUG -Wno-unused-variable -c -o obj/token.o src/token.cpp -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk --sysroot=/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk -isystem/opt/homebrew/include -isystem/Library/Developer/CommandLineTools/SDKs/MacOSX13.sdk/System/Library/Frameworks/OpenGL.framework/Versions/Current/Headers -I/opt/homebrew/opt/icu4c@77/include -I/opt/homebrew/opt/llvm@16/include -fdebug-prefix-map=/private/tmp/nesfab-20250316-8709-5r20tf/nesfab-da18a43dc7b941cc4c56c949303aa37633fdc1b0=.
  
  Xcode and/or the CLT appear to be misconfigured. Try one or both of the following:
    xcodebuild -license
    sudo xcode-select --switch /path/to/Xcode.app

Yep, a new hell 😁 because a different thing breaks when I use llvm@latest, so I’m trying to use llvm@16 to match what works on macOS 14 & 15. I had to go to bed so didn’t get a chance to troubleshoot this one last night.

@vtbassmatt vtbassmatt force-pushed the add-nesfab branch 8 times, most recently from 42544d8 to 57533dc Compare March 17, 2025 11:27
@vtbassmatt
Copy link
Author

😭 OK, I'm crying uncle. I cannot seem to get this building on macOS 13.

  1. It builds with gcc, but then doesn't link because its dependency (Boost) was built with Clang
  2. It doesn't build with the macOS 13 default Clang because std::lexicographical_compare_three_way wasn't part of that libc++
    • No solution found: I don't think upstream will accept removing this call. It's part of the C++20 standard, and apparently offers significant runtime savings. Older Clang was not fully implementing the standard yet.
  3. It doesn't build with depends_on: 'llvm' (which is @19) because that libc++ removed char_traits<unsigned char>
    • Possible solution: I'm working with upstream to get this fixed. There may still be an onion-peeling exercise to see what else has dropped/changed between Xcode's LLVM on Sequoia and bleeding-edge LLVM.
  4. It doesn't build with depends_on: 'llvm@18' (or 17 or 16) because every compiler invocation fails with:
Xcode and/or the CLT appear to be misconfigured. Try one or both of the following:
  xcodebuild -license
  sudo xcode-select --switch /path/to/Xcode.app"

Number 4 perplexes me the most. It actually seems like the correct path forward, if someone can help me understand what I need to do in the Homebrew CI to get this fixed.

  • xcodebuild -license requires user-at-keyboard (I think? I couldn't figure out how to do it headless).
  • I don't know where to tellxcode-select to look for some different Xcode.
  • I tried adding the LDFLAGS suggested in https://formulae.brew.sh/formula/llvm@18, but that didn't fix anything.

Help, please 🙏

@botantony
Copy link
Contributor

@vtbassmatt instead of LDFLAGS try to use this:

ENV.prepend_path "HOMEBREW_LIBRARY_PATHS", Formula["llvm@18"].opt_lib/"c++"

@vtbassmatt vtbassmatt force-pushed the add-nesfab branch 2 times, most recently from f3c715d to e8b4988 Compare March 17, 2025 14:15
@vtbassmatt
Copy link
Author

@vtbassmatt instead of LDFLAGS try to use this:

ENV.prepend_path "HOMEBREW_LIBRARY_PATHS", Formula["llvm@18"].opt_lib/"c++"

Thanks. Gave it a try, and it still failed in the same way. I've cribbed some stuff from the llvm@18 test stanza that I'm trying now:

      toolchain_path = "/Library/Developer/CommandLineTools"
      cpp_base = (MacOS.version >= :big_sur) ? MacOS::CLT.sdk_path : toolchain_path

      system "make", "GIT_COMMIT=#{git_sha}-homebrew", "CXX=#{ENV.cxx}",
             "-isysroot", MacOS::CLT.sdk_path,
             "-isystem", "#{cpp_base}/usr/include/c++/v1",
             "-isystem", "#{MacOS::CLT.sdk_path}/usr/include",
             "-isystem", "#{toolchain_path}/usr/include",
             "release"

though I'm not sure what Make will do with those -isys* lines.

@SMillerDev
Copy link
Member

It doesn't build with the macOS 13 default Clang because std::lexicographical_compare_three_way llvm/llvm-project#68591

Seems like the easiest way is declaring that it needs at least macOS 14 to build.

@vtbassmatt
Copy link
Author

It doesn't build with the macOS 13 default Clang because std::lexicographical_compare_three_way llvm/llvm-project#68591

Seems like the easiest way is declaring that it needs at least macOS 14 to build.

That's the conclusion I've arrived at as well 🤣. I'm going to try dropping Ventura before I pull any more hair out.

@vtbassmatt
Copy link
Author

vtbassmatt commented Mar 17, 2025

Appreciate everyone's help getting here! Yup, it turned out that I just needed to say "so long" to Ventura / macOS 13. It builds on 14+.

@vtbassmatt
Copy link
Author

@SMillerDev sorry to bother you - do I need to do anything else to have this contribution accepted?

Comment on lines 4 to 6
# for this version only, point to a specific commit. post-1.6, this will point to a tagged release.
url "https://github.com/pubby/nesfab/archive/da18a43dc7b941cc4c56c949303aa37633fdc1b0.tar.gz"
version "1.6"
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to wait for a tagged release

Copy link
Author

Choose a reason for hiding this comment

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

OK, I will ask upstream if they can do a small release so we have something to point to.

Comment on lines 32 to 35
system "make", "clean"
system "make", "GIT_COMMIT=#{git_sha}-homebrew", "CXX=#{ENV.cxx}", "debug" if Hardware::CPU.intel?
system "make", "GIT_COMMIT=#{git_sha}-homebrew", "CXX=#{ENV.cxx}", "ARCH=", "debug" if Hardware::CPU.arm?
bin.install "nesfab"
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to build debug builds

Copy link
Author

@vtbassmatt vtbassmatt Mar 24, 2025

Choose a reason for hiding this comment

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

This formula is for a compiler. The debug flavor inserts runtime checks into the generated code to help with the development experience, while the release flavor doesn't since the target system is very resource-constrained. It's definitely unusual that upstream chose to make these two different builds rather than an option to a single binary, but refactoring that is beyond the scope of adding to a package manager. Both are necessary for a full development environment.

Is there another naming convention I could use for the binaries, or some other way to make this fit the spirit of the Homebrew policy without major surgery on upstream's architectural choice?

Copy link
Member

Choose a reason for hiding this comment

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

if both flavors are useful, we can probably install both. (this also sounds like static/dynamic shared lib)

Copy link
Author

Choose a reason for hiding this comment

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

OK, great. If this aspect won't be a blocker, I believe that's the last open question from Homebrew. I'll see if upstream is willing to cut a release, I can point to it, and maybe get this thing merged!

if OS.mac?
system "make", "GIT_COMMIT=#{git_sha}-homebrew", "CXX=#{ENV.cxx}", "release" if Hardware::CPU.intel?
system "make", "GIT_COMMIT=#{git_sha}-homebrew", "CXX=#{ENV.cxx}", "ARCH=", "release" if Hardware::CPU.arm?
bin.install "nesfab" => "nesfab-release"
Copy link
Member

Choose a reason for hiding this comment

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

Is there no make install?

Copy link
Author

Choose a reason for hiding this comment

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

There is not. Upstream builds a single binary and so recommends people to simply build the thing and copy it into someplace in $PATH.

Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Apr 14, 2025
@vtbassmatt
Copy link
Author

Not stale. Waiting until upstream does a point release that I can rebase to.

@github-actions github-actions bot removed the stale No recent activity label Apr 14, 2025
@daeho-ro daeho-ro added the in progress Stale bot should stay away label Apr 16, 2025
@chenrui333
Copy link
Member

upstream has made a 1.6_mac release, https://github.com/pubby/nesfab/releases/tag/v1.6_mac?

@vtbassmatt
Copy link
Author

upstream has made a 1.6_mac release, https://github.com/pubby/nesfab/releases/tag/v1.6_mac?

Unfortunately it didn’t have everything needed to land a working Homebrew package.

Enough time has elapsed now that perhaps the maintainer can be persuaded to do another point release. I was waiting to see if anyone had more thoughts on #211113 (comment) though.

@chenrui333
Copy link
Member

Enough time has elapsed now that perhaps the maintainer can be persuaded to do another point release.

yeah, probably, also better to move the installation process into the upstream as well (like one goal to install both flavors)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
boost Boost use is a significant feature of the PR or issue in progress Stale bot should stay away new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants