-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add macOS CI #120
Add macOS CI #120
Conversation
Yeah, I'll definitely need @tsjensen to help with this. I know I'm doing something dumb... :) |
Well, I've gotten closer. First, I had to install the GNU versions of sed, grep, and xargs since the BSD ones that macOS has by default are odd and your tests use GNU options (and I don't mind this because I hate the BSD versions of those utilities). But even with that, as far as I can tell the white-box tests can't run at the moment since macOS ld does not support --wrap and I'm not sure how to get around that. Frankly, I'm not sure what it's doing. 😄 Now the black-box tests get pretty much through everything...but dies at the end with an error. I can see one test fails for Unicode/language-y (
Internet said setting I also see:
and then finally:
|
That's great progress! Here's my 2 cents on some of the topics:
|
Don't know if this is helpful, or if you've already worked this out, but on my M1 Mac Mini, I tried to figure out if there was a way to simulate the Finally, I decided to try the "nuclear option", and was able to get the unit tests to pass by defining a function pointer (I also added a section in flags_osx:
$(eval CFLAGS := -I. -I$(SRC_DIR) -O -Wall -W -Wno-stringop-overflow $(CFLAGS_ADDTL))
$(eval LDFLAGS := $(LDFLAGS) --coverage $(LDFLAGS_ADDTL))
$(eval UTEST_EXECUTABLE_NAME := unittest)
$(eval UTEST_OBJ := $(UTEST_NORM:.c=.o)) If this is helpful, I'd be glad to submit a pull request with my changes (or maybe an update this pull request? ... I'm not sure of the proper way to do this kind of thing). Here's the output of
|
Incidentally, I haven't yet merged in @mathomp4's changes from this mathomp4:add-macos-ci branch -- I created my branch off of the color-unicode branch, since the whole reason I wanted to build from source was to get ANSI box drawing characters to work... which they do (although the terminal font has a big effect on how nice they end up looking): $ echo "boxes is great!" | out/boxes -d ansi
┌─────────────────┐
│ boxes is great! │
└─────────────────┘
$ echo "boxes is great!" | out/boxes -d ansidouble
╔═════════════════╗
║ boxes is great! ║
╚═════════════════╝
$ echo "boxes is great!" | out/boxes -d ansirounded
╭─────────────────╮
│ boxes is great! │
╰─────────────────╯
$ echo "boxes is great!" | out/boxes -d ansiheavy
┏━━━━━━━━━━━━━━━━━┓
┃ boxes is great! ┃
┗━━━━━━━━━━━━━━━━━┛
$ echo "boxes is great!" | out/boxes -d ansidashed
┌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┐
┊ boxes is great! ┊
└╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┘
$ |
@chorpler That looks very promising. Thank you for sharing! We would much appreciate you contributing this. It seems to me that your changes and @mathomp4's should work together nicely. Maybe you could just update this PR? I would then rebase the color-unicode branch on top of it, so we can already build it on Mac. |
It might be that updating this PR is not so straightforward because its content is from another fork. So, please feel free to open another PR if that's easier for you. 🙂 |
@chorpler I'm willing to do whatever you'd like to get this in. You can either fork my fork and make a PR to my branch. Or, since my changes are so small, if you make a PR with your changes, I can merge your branch into this branch and we can see if the macos CI works with our combined power! |
Had some "fun" and added a bit of ugly bash to work around test 111 on macOS. Also fixed up the coveralls since we now have two checks instead of one. I also added an "ignore error" for the issue that will probably be fixed by the work of @chorpler . But I wanted to make sure the full workflow would work if we had everything working. 😄 |
Last night I tried to figure out the best way to add my changes. My first attempt at cherry-picking commits to merge into @mathomp4's branch ended up pulling in some changes from the color-unicode branch (from the same files I'd made changes in) and rendered the whole thing completely unbuildable. :D I ended up creating a branch from the mathomp4:add-macos-ci branch and just redoing my changes on top of it. Which was good, because I realized that if I called the function pointer Then I checked that things would still build and run on Linux and Windows.
Anyway, I'll go ahead and create the pull request to merge my new branch into the existing mathomp4:add-macos-ci branch, and we'll go from there. |
I created a pull request here: mathomp4#1 Hopefully that will allow this pull request to be updated also. If it doesn't work, we can create a new pull request. |
I've pulled in the branch from @chorpler into my branch. Good news: macOS passes both white-box and black-box tests! 🎉 Bad news: Linux is now failing white-box with segfaults and other stuff 😞 : https://github.com/ascii-boxes/boxes/actions/runs/6997063497/job/19033607396 I even tried adding: env:
LEX: flex
YACC: bison to the action in case I hit the Fedora issue @chorpler saw above, but it didn't change anything. I'm a Fortran programmer by trade so I'm not great at debugging C. Hopefully you all can see what's happening! |
OK, yeah, I see what I did. While redoing my changes in the I've submitted a new pull request for your mathomp4:add-macos-ci branch, @mathomp4. It looks like the |
It occurs to me that as an alternative to changing line in |
Undrafted! 🎉 |
I would appreciate if you removed the |
Sounds good, I'll remove it. Thanks for the tip on Windows bison using |
21d7a90
to
81103fc
Compare
One more thing, a bit about cosmetics ... I am a big fan of a linear commit history in my projects (no merge commits). So, I would like to do some cleanup of the commit history on this branch before I merge it, also rebasing it onto the latest master and so on. (Content will not be changed, of course.) I would offer to do that myself, but only if it's OK with you. 😊 If you like, I can also merge mathomp4#3 while I'm at it. |
Huh. I didn't see a new PR. Good job GitHub notifications. Let me do that real quick. And then after that, it's up to you. But first, merge time... |
Okay. The PR from @chorpler is merged in. |
…`__wrap_bx_fprintf()`
Remove flags_darwin, which are the same as flags_unix now
858044a
to
f1bda75
Compare
Ok, now we have nice, linear commits. 😸 I also made another small change regarding the defaults for |
This is attempting to add macOS in the CI.