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

[libmicrohttpd] Add support for Windows ARM builds #31088

Closed

Conversation

anmatako
Copy link

@anmatako anmatako commented Apr 24, 2023

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@anmatako
Copy link
Author

@microsoft-github-policy-service agree company="Microsoft"

@anmatako anmatako marked this pull request as ready for review April 24, 2023 17:19
#define W32_VARDECORPREFIX _
#define W32_DECORVARNAME(v) _ ## v
#define W32_VARDECORPEFIXSTR "_"
diff --git a/w32/VS2015/hellobrowser.vcxproj b/w32/VS2015/hellobrowser.vcxproj
Copy link
Author

Choose a reason for hiding this comment

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

This patch is minimally modifying only the VS2015 solution that actually gets built by the port. The other flavors for VS2017, 2019, and 2022 can be updated similarly if there's desire to build one of these instead.

@BillyONeal
Copy link
Member

Has this change been submitted upstream?

@anmatako
Copy link
Author

Has this change been submitted upstream?

I'm not sure I understand the question. It's my first time opening a PR for vcpkg. I though that submitting the PR to the repo is what I was supposed to do for the upstream.

@BillyONeal
Copy link
Member

I'm not sure I understand the question. It's my first time opening a PR for vcpkg. I though that submitting the PR to the repo is what I was supposed to do for the upstream.

Ah, sorry for the confusion. This seems like a bugfix to the build system of libmicrohttpd; there's no reason that the fix should be specific to vcpkg. In general we like to make sure upstream, in this case the folks who make libmicrohttpd, are at least notified when there are not specific-to-vcpkg patches like this. It helps avoid situations like https://wiki.debian.org/SSLkeys#End_User_Summary where Debian added OpenSSL patches that broke key generation.

Because this seems to only touch build system stuff we don't necessarily need to wait for those folks to have merged the changes before proceeding, but we would prefer if they were at least notified.

@jimwang118 jimwang118 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Apr 25, 2023
@anmatako
Copy link
Author

I'm not sure I understand the question. It's my first time opening a PR for vcpkg. I though that submitting the PR to the repo is what I was supposed to do for the upstream.

Ah, sorry for the confusion. This seems like a bugfix to the build system of libmicrohttpd; there's no reason that the fix should be specific to vcpkg. In general we like to make sure upstream, in this case the folks who make libmicrohttpd, are at least notified when there are not specific-to-vcpkg patches like this. It helps avoid situations like https://wiki.debian.org/SSLkeys#End_User_Summary where Debian added OpenSSL patches that broke key generation.

Because this seems to only touch build system stuff we don't necessarily need to wait for those folks to have merged the changes before proceeding, but we would prefer if they were at least notified.

OK I see what you mean now :). Given how this library is maintained it's hard to figure out if there's even a plan to support ARM builds. I don't see any open issues regarding that or a way to submit a PR for their build system. I understand if you're unwilling to take this change in VCPKG and I can find an alternative path forward for the use-case I'm trying to cover on my end,

@anmatako
Copy link
Author

@BillyONeal I will notify the microhttpd mailing list regarding the ARM support and the related change posted here. Seeing that a change from 0.9.75 to 0.9.76 took over a year, a response and/or acknowledgement may take a while :)

@Karlson2k
Copy link
Contributor

The proper fix will be provided by the upstream.
You can close this PR.

@BillyONeal
Copy link
Member

@BillyONeal I will notify the microhttpd mailing list regarding the ARM support and the related change posted here. Seeing that a change from 0.9.75 to 0.9.76 took over a year, a response and/or acknowledgement may take a while :)

Sure, like I said, we don't absolutely need them to have responded with a release or similar, but we do want to see that they at least have been notified and haven't explicitly said 'no we don't want that' or similar.

The proper fix will be provided by the upstream. You can close this PR.

Is it expected that they'll drop a release for that or should we just add whatever patch they decided to do?

@anmatako
Copy link
Author

@BillyONeal I just reached out to the mailing list yesterday and have not heard back yet. I'll wait to see if there is a response on their end with willingness (or unwillingness) to take the change on their end. In the case the upstream is not interested in supporting an ARM build, will this be acceptable to take this change into VCPKG or will the path forward be blocked?

@BillyONeal
Copy link
Member

@BillyONeal I just reached out to the mailing list yesterday and have not heard back yet. I'll wait to see if there is a response on their end with willingness (or unwillingness) to take the change on their end. In the case the upstream is not interested in supporting an ARM build, will this be acceptable to take this change into VCPKG or will the path forward be blocked?

I think we should give them 30 days to respond but if they don't we'll take the patch. Is there a link to a mailing list archive or something we can record here?

@BillyONeal BillyONeal added the depends:upstream-changes Waiting on a change to the upstream project label Apr 25, 2023
@anmatako
Copy link
Author

anmatako commented Apr 26, 2023

@BillyONeal I just reached out to the mailing list yesterday and have not heard back yet. I'll wait to see if there is a response on their end with willingness (or unwillingness) to take the change on their end. In the case the upstream is not interested in supporting an ARM build, will this be acceptable to take this change into VCPKG or will the path forward be blocked?

I think we should give them 30 days to respond but if they don't we'll take the patch. Is there a link to a mailing list archive or something we can record here?

One thing I did not realize till later when I saw the mailing list digest is that @Karlson2k is involved in microhttpd and willing to take the upstream changes needed to enable the ARM builds. Thank you @Karlson2k for that!

@BillyONeal given that, I think I will leave this PR open until the upstream is updated and then I will update the port with the correct version bump for libmicrohttpd with ARM support.

Does that sound reasonable?

BTW this is the mailing list archive for reference https://lists.gnu.org/archive/html/libmicrohttpd/2023-04/msg00003.html

@BillyONeal BillyONeal marked this pull request as draft April 26, 2023 18:44
@BillyONeal
Copy link
Member

I think I will leave this PR open until the upstream is updated

Sounds good, just un-draft the PR when you are ready by clicking the 'ready for review' button :)

@Karlson2k
Copy link
Contributor

Is it expected that they'll drop a release for that or should we just add whatever patch they decided to do?

I'm the upstream maintainer.
The new release will be made in foreseeable future.

The support will be implemented in git master soon.

You can wait for the new release (with other fixes) or just cherry-pick commits for ARM/ARM64 support in VS project files.

BTW I see other problems in current portfile: wrong set of headers used, completely disabled ARM and others. ARM is fully supported when libmicrohttpd built by GCC or clang. Only MSVC implementation is not complete.
Probably I'll open PR to fix these and other problems.

@Karlson2k
Copy link
Contributor

The Visual Studio with ARM/ARM64 support has been added in git master. The diff:

After some testing it will be pulled to the next version branch and then published.

@anmatako
Copy link
Author

anmatako commented May 2, 2023

The Visual Studio with ARM/ARM64 support has been added in git master. The diff:

After some testing it will be pulled to the next version branch and then published.

@Karlson2k Thank you so much for the upstream update. I'm trying it out now by taking your patch into my vcpkg fork so I can build it through the pipeline and test it out. It will take a day or two to find the time to test the ARM builds, but I will let you know as soon as this is done.

@anmatako
Copy link
Author

anmatako commented May 5, 2023

The Visual Studio with ARM/ARM64 support has been added in git master. The diff:

After some testing it will be pulled to the next version branch and then published.

@Karlson2k Thank you so much for the upstream update. I'm trying it out now by taking your patch into my vcpkg fork so I can build it through the pipeline and test it out. It will take a day or two to find the time to test the ARM builds, but I will let you know as soon as this is done.

FYI @Karlson2k I was able to confirm the ARM binaries of libmicrohttpd through our internal testing. Let us know when you get the changes merged into the upstream and there's a new version out.

@Karlson2k
Copy link
Contributor

Karlson2k commented May 7, 2023

FYI @Karlson2k I was able to confirm the ARM binaries of libmicrohttpd through our internal testing. Let us know when you get the changes merged into the upstream and there's a new version out.

Thanks for the testing and for the confirmation, @anmatako.
The changes are already merged upstream. The new version should be released in one or two weeks.

@jimwang118
Copy link
Contributor

@anmatako The upstream has incorporated the patch that supports arm, you can update the upstream version to complete this PR.

@Karlson2k
Copy link
Contributor

Actually the fixed version was already merged via PR #31740 so probably this PR should be closed.

@jimwang118
Copy link
Contributor

Duplicate of #31740

@jimwang118 jimwang118 marked this as a duplicate of #31740 Jun 16, 2023
@jimwang118 jimwang118 closed this Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist depends:upstream-changes Waiting on a change to the upstream project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants