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

xhyve: bump version to support Big Sur #11911

Merged
merged 1 commit into from
Sep 23, 2021
Merged

Conversation

catap
Copy link
Contributor

@catap catap commented Aug 17, 2021

Description

This is moving to a few commits ahead mainly to include machyve/xhyve#200 which introduced support of Big Sur

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 11.5.1 20G80 x86_64
Xcode 12.5.1 12E507

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL?
  • checked your Portfile with port lint?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?

@macportsbot
Copy link

Notifying maintainers:
@jeremyhu for port xhyve.

@catap catap force-pushed the xhyve branch 2 times, most recently from 1317635 to c7d7f76 Compare August 17, 2021 20:58
@catap catap requested a review from reneeotten August 17, 2021 20:59
@catap
Copy link
Contributor Author

catap commented Aug 17, 2021

About CI: it fails with very wired issue:

2021-08-17T21:01:38.6978470Z While building module 'Hypervisor' imported from /opt/local/var/macports/build/_Users_runner_work_macports-ports_macports-ports_ports_emulators_xhyve/xhyve/work/machyve-xhyve-83516a0/src/vmm/intel/vmx_msr.c:34:
2021-08-17T21:01:38.6980060Z In file included from <module-includes>:1:
2021-08-17T21:01:38.6981230Z /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Hypervisor.framework/Headers/hv_vmx.h:67:35: error: expected function body after function declarator
2021-08-17T21:01:38.6982310Z         uint32_t field, uint64_t *value) __HV_10_15;
2021-08-17T21:01:38.6982700Z                                          ^
2021-08-17T21:01:38.6984050Z /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Hypervisor.framework/Headers/hv_vmx.h:82:34: error: expected function body after function declarator
2021-08-17T21:01:38.6985190Z         uint32_t field, uint64_t value) __HV_10_15;
2021-08-17T21:01:38.6985570Z                                         ^
2021-08-17T21:01:38.6986580Z /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Hypervisor.framework/Headers/hv_vmx.h:109:43: error: expected function body after function declarator
2021-08-17T21:01:38.6987860Z         uint32_t field, hv_shadow_flags_t flags) __HV_10_15;
2021-08-17T21:01:38.6988270Z                                                  ^
2021-08-17T21:01:38.6988640Z 3 errors generated.
2021-08-17T21:01:38.6990270Z /opt/local/var/macports/build/_Users_runner_work_macports-ports_macports-ports_ports_emulators_xhyve/xhyve/work/machyve-xhyve-83516a0/src/vmm/intel/vmx_msr.c:34:10: fatal error: could not build module 'Hypervisor'
2021-08-17T21:01:38.6991390Z #include <Hypervisor/hv.h>
2021-08-17T21:01:38.6991760Z  ~~~~~~~~^
2021-08-17T21:01:38.7039370Z 4 errors generated.

This macros is defined like this:

#define __HV_10_10 __OSX_AVAILABLE_STARTING(__MAC_10_10, __IPHONE_NA)
#define __HV_10_15 __OSX_AVAILABLE_STARTING(__MAC_10_15, __IPHONE_NA)

and I feel that a root cause of this issue is github's virtualisation.

@reneeotten
Copy link
Contributor

I don't know anything about this port nor really understand the error message you show... so I'm afraid that I cannot be if much help for this.

@catap
Copy link
Contributor Author

catap commented Aug 18, 2021

@reneeotten seems that it is "feature" of clang 12 on macOS 10.15.

I have create he simpler possible reproducer for this error:

catap@catapsmCatalina /tmp % cat test.c 
#include <Hypervisor/hv.h>


catap@catapsmCatalina /tmp % clang -fmodules test.c
While building module 'Hypervisor' imported from test.c:1:
In file included from <module-includes>:1:
/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Hypervisor.framework/Headers/hv_vmx.h:67:35: error: expected function body after function
      declarator
        uint32_t field, uint64_t *value) __HV_10_15;
                                         ^
/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Hypervisor.framework/Headers/hv_vmx.h:82:34: error: expected function body after function
      declarator
        uint32_t field, uint64_t value) __HV_10_15;
                                        ^
/Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/System/Library/Frameworks/Hypervisor.framework/Headers/hv_vmx.h:109:43: error: expected function body after function
      declarator
        uint32_t field, hv_shadow_flags_t flags) __HV_10_15;
                                                 ^
test.c:1:10: fatal error: could not build module 'Hypervisor'
#include <Hypervisor/hv.h>
 ~~~~~~~~^
4 errors generated.
catap@catapsmCatalina /tmp % clang --version
Apple clang version 12.0.0 (clang-1200.0.32.29)
Target: x86_64-apple-darwin19.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
catap@catapsmCatalina /tmp % 

If using clang-11 it works as expected. clang-12 introduced the full modules support as -fmodules and an old one is renamed to -fmodules-ts.

Anyway, I have no idea how to replace -fmodules to -fmodules-ts on xcode project based build such we have here.

Maybe you can addvice or tag someone who can?

Thanks!

@catap catap force-pushed the xhyve branch 4 times, most recently from 6d03719 to 96229f0 Compare August 20, 2021 14:02
@catap
Copy link
Contributor Author

catap commented Aug 20, 2021

Wired. I can build it on:

macOS 10.15.7 19H15 x86_64
Xcode 12.4 12D4e

Copy link
Contributor

@reneeotten reneeotten left a comment

Choose a reason for hiding this comment

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

@catap as I said earlier I'm sorry but I don't have the expertise to help you out on this. You could ask help on the mailinglist - there are likely people out there who would understand what's going on.

@catap
Copy link
Contributor Author

catap commented Sep 2, 2021

@reneeotten can it be merged without passing some old system? :)

@reneeotten
Copy link
Contributor

@reneeotten can it be merged without passing some old system? :)

I think the "policy" is to support the latest three macOS releases, anything older isn't hard requirement but we do attempt to support them as much as possible. This does not build on the CI for 10.15 so that's likely something that needs to be fixed before we'd consider merging the PR.

@catap
Copy link
Contributor Author

catap commented Sep 2, 2021

@reneeotten and if the old port doesn't build on modern CI system? The issue is Xcode 12.4? I'd like to revert all my changes to just bump a revision number to proof that ;)

@catap
Copy link
Contributor Author

catap commented Sep 2, 2021

@reneeotten you may see that CI fails on the same issue on current code. I have no idea how to fix it, because the bug is inside Xcode 12.4 :(

@reneeotten
Copy link
Contributor

@kencu @cjones051073 has any of you seen this issue before and/or is able to take a look? I'm afraid this is above my pay-grade and I cannot really help here anymore...

@kencu
Copy link
Contributor

kencu commented Sep 3, 2021

A mismatch between headers that are pulled in would seem to be likely at play. Exactly what mismatch I am not yet clear on, but __HV_10_15 is supposed to be defined but is not being defined.

A similar issue with modules occurred with the ncurses port. As I recall how it worked (18 months ago) when the modules are being used the build system did not follow the expected order of including headers, and in ncurses, the wrong (system) header was pulled in instead of the one we wanted to use (in /opt/local/include).

https://trac.macports.org/ticket/59992

the temporary solution, at least, was to add a module.map for ncurses that was installed in /opt/local/include that would service ncurses.

d2c8da9#diff-4f1aa1994e32ae4d841cca3766dec0d31cd5c4cd6aeeb07dd114bc9b1b49a4cb

It was obvious that this was going to be the tip of the iceberg for this issue, and ncurses was only going to be the first of many such modules issues. How that was ever going to be coherently sorted out for the general case was unknown.

@catap
Copy link
Contributor Author

catap commented Sep 8, 2021

@reneeotten hello here!

I have two open questions about this PR / port:

  • can it be merged? :)
  • because I've started to claim maintainership for some ports I can also do for this one which I'm using. Anyway, I haven't see any explanation how to do it when a port has maintainer.

@catap
Copy link
Contributor Author

catap commented Sep 15, 2021

@pmetzger can I ask your thought about this one?

Current version can't be build on both: 10.15 and 11 :) This one fixed it.

@kencu
Copy link
Contributor

kencu commented Sep 17, 2021

apologies this is taking a bit of time.

I presume you already have it locally installed, so you are using it now. So you're all set. This PR is for the general population out there, and thanks for that contribution.

There seem to be a few things that will need verifying that the fixes are the ones needed long-term to support general users, and it will take a bit of time for that to happen.

Please don't be frustrated while it is looked over.

@pmetzger
Copy link
Member

It looks like we're now at the CI passing and Jeremy has clearly abdicated management of this port (it might be reasonable to open a "port abandoned" ticket fwiw.) I'd suggest that if no one has anything significant to say that we merge this in the next few days.

@pmetzger
Copy link
Member

@catap btw, you should probably apply for commit privileges. You're certainly working on MacPorts more than enough for that.

@catap
Copy link
Contributor Author

catap commented Sep 17, 2021

@pmetzger I've already started the process about take care of this port: https://trac.macports.org/ticket/63495

and regarding commit privileges, I'd like to agree with you. But I plan to do it when #12132 is merged.

emulators/xhyve/files/add-hypervisor-elements.diff Outdated Show resolved Hide resolved
emulators/xhyve/files/disable-VMX-big-sur.diff Outdated Show resolved Hide resolved
@catap
Copy link
Contributor Author

catap commented Sep 22, 2021

@jeremyhu, as you make clear in https://trac.macports.org/ticket/63495 I've made myself also maintaier of this port.

Thus, the new version contains small fixes for clang clang-1300.0.29.3 which is shiped with 11.6. A patch was backported to upstream: machyve/xhyve#219

@jeremyhu let me know if you plan to merge the patch to upstream to make one more bump :)

Copy link
Member

@pmetzger pmetzger left a comment

Choose a reason for hiding this comment

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

One small comment.

emulators/xhyve/Portfile Outdated Show resolved Hide resolved
Copy link
Member

@pmetzger pmetzger left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -0,0 +1,44 @@
commit 55e2b427882624157cbce08a211c5af83043bb2f
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you upstream this as well? I didn't see it when quickly looking through the pending pull requests upstream last night (I could have missed it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeremyhu I'm usually send to upstream all patches that I'm making. For example this one is here: machyve/xhyve#219

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, it is quite new. Because without this patch I can compile it on Big Sur until 11.6.

@pmetzger
Copy link
Member

Thanks, @catap, and do continue to monitor for updates that might do things like incorporating your patches.

@catap catap deleted the xhyve branch September 23, 2021 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants