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

Try vm-actions instead of cross-platform-actions #589

Merged
merged 1 commit into from
Feb 20, 2023
Merged

Try vm-actions instead of cross-platform-actions #589

merged 1 commit into from
Feb 20, 2023

Conversation

davidchisnall
Copy link
Collaborator

This uses VirtualBox instead of xhyve. It might be slower, but should be more reliable.

@davidchisnall davidchisnall force-pushed the vm-ci branch 10 times, most recently from e7dca1c to 8618032 Compare February 10, 2023 12:49
This uses VirtualBox instead of xhyve.  It might be slower, but should
be more reliable.

Tests run on FreeBSD, NetBSD, and OpenBSD.  Only the FreeBSD ones are
passing at the moment, the others will keep running but aren't added as
dependencies for the action used to guard commits.
@davidchisnall
Copy link
Collaborator Author

This is running the tests on {Net,Free,Open}BSD. The tests currently fail on Net- and OpenBSD, though for different reasons. I've wired up FreeBSD to the all-checks action, but not the other two, so the failing tests don't break merging but it's easy to see if a commit fixes them (once they're fixed, hopefully we can keep them from breaking again).

@davidchisnall
Copy link
Collaborator Author

@devnexen, it looks as if some of the tests fail on NetBSD as well.

@@ -288,7 +288,7 @@ if(NOT SNMALLOC_HEADER_ONLY_LIBRARY)

if(NOT (DEFINED SNMALLOC_LINKER_FLAVOUR) OR ("${SNMALLOC_LINKER_FLAVOUR}" MATCHES "^$"))
# Linker not specified externally; probe to see if we can make lld work
set(CMAKE_REQUIRED_LINK_OPTIONS -fuse-ld=lld)
set(CMAKE_REQUIRED_LINK_OPTIONS -fuse-ld=lld -Wl,--icf=all)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove this? It makes the library smaller as there are a lot of names for the same function in the API surface (e.g. lots of new overloads and malloc can all be folded into a single thing).

Copy link
Member

Choose a reason for hiding this comment

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

FYI: this change increases the size of libsnmallocshim.so by about 10%.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't remove it, I added it to the check that enables it. Without this, we now have CI that triggers the reason that I have to carry a local patch for snmalloc to disable this: If you compile with GCC, the -fuse-ld=lld test fails (GCC consumes but silently ignores the flag? Or just warns and doesn't error?), but then GCC passes this flag to ld.bfd and it fails to link.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I miss read the diff.

@devnexen
Copy link
Collaborator

devnexen commented Feb 10, 2023

@devnexen, it looks as if some of the tests fail on NetBSD as well.

it comes down to the access enforcing thing. Once disabled all pass. If you do not mind I can commit to your branch or maybe you prefer to handle it alone.

@davidchisnall
Copy link
Collaborator Author

@devnexen, please feel free to push to this branch or raise a PR against it.

@mjp41
Copy link
Member

mjp41 commented Feb 20, 2023

Can we merge this to unblock the CI?

@mjp41 mjp41 self-requested a review February 20, 2023 12:05
@mjp41 mjp41 merged commit b9b9055 into main Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants