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

[libmesh] Fix Windows build #22810

Closed
wants to merge 1 commit into from
Closed

[libmesh] Fix Windows build #22810

wants to merge 1 commit into from

Conversation

mkhon
Copy link
Contributor

@mkhon mkhon commented Jan 27, 2022

Describe the pull request

  • update to the master w/ Windows build patches

  • What does your PR fix?

libmesh build on Windows

  • Which triplets are supported/not supported? Have you updated the CI baseline?

Yes. I haven't changed ci.baseline.txt as there are still issues with conflicting files installed by libmesh port which should be handled in a separate PR

Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libmesh/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libmesh/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

@mkhon
Copy link
Contributor Author

mkhon commented Jan 27, 2022

Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

Please remove all the content related to libmesh:WIN_TRIPLET in VCPKG_ROOT/scripts/ci.baseline.txt.

@JackBoosY JackBoosY added 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 requires:author-response labels Jan 27, 2022
@JackBoosY
Copy link
Contributor

Waiting for the upstream approval.

@mkhon
Copy link
Contributor Author

mkhon commented Jan 27, 2022

Please remove all the content related to libmesh:WIN_TRIPLET in VCPKG_ROOT/scripts/ci.baseline.txt.

@JackBoosY this requires more work as the port installs conflicting files:

The following files are already installed in C:/vcpkg/installed/x64-windows-release and are in conflict with libmesh:x64-windows-release

Installed by netcdf-c:x64-windows-release
    include/netcdf.h
    include/netcdf_aux.h
    include/netcdf_mem.h
    include/netcdf_meta.h
    lib/libnetcdf.settings
    lib/netcdf.lib
    lib/pkgconfig/netcdf.pc

And there are more (see scripts/ci.baseline.txt entries for this port)

@JackBoosY
Copy link
Contributor

Please remove all the content related to libmesh:WIN_TRIPLET in VCPKG_ROOT/scripts/ci.baseline.txt.

@JackBoosY this requires more work as the port installs conflicting files:

The following files are already installed in C:/vcpkg/installed/x64-windows-release and are in conflict with libmesh:x64-windows-release

Installed by netcdf-c:x64-windows-release
    include/netcdf.h
    include/netcdf_aux.h
    include/netcdf_mem.h
    include/netcdf_meta.h
    lib/libnetcdf.settings
    lib/netcdf.lib
    lib/pkgconfig/netcdf.pc

And there are more (see scripts/ci.baseline.txt entries for this port)

So you should change fail to skip in ci.baseline.txt and add comment.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libmesh/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libmesh/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

@mkhon
Copy link
Contributor Author

mkhon commented Jan 27, 2022

So you should change fail to skip in ci.baseline.txt and add comment.

@JackBoosY have you checked the source code before giving the advices and marking the port as requires:author-response?

The port is already marked as 'skip' in ci.baseline.txt with the appropriate comments

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libmesh/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

@JackBoosY
Copy link
Contributor

So you should change fail to skip in ci.baseline.txt and add comment.

@JackBoosY have you checked the source code before giving the advices and marking the port as requires:author-response?

The port is already marked as 'skip' in ci.baseline.txt with the appropriate comments

Ah sorry, they were marked skip. This must be my memory messed up.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libmesh/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libmesh/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libmesh/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libmesh/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libmesh/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

- compile .C files as C++
- Fix unistd.h inclusions
- export explicit template instantiations as libtool "export all" linking
  does not pick them up due to their sections marked as "(pick any)"
  in dumpbin output
- other MS VC-related fixes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/libmesh/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response depends:upstream-changes Waiting on a change to the upstream project labels Feb 5, 2022
@JackBoosY
Copy link
Contributor

The upstream approved this changes.

Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I see that upstream has approved and merged libMesh/libmesh#3135, and tacitly has approved libMesh/libmesh#3144, however there is still a significant amount of patching in this PR that hasn't been submitted upstream (as far as I can tell).

I'd really like to see the patches in this PR reduced to pulling diffs from the libmesh GitHub, for example:

vcpkg_download_distfile(
    PATCH1
    URLS "https://patch-diff.githubusercontent.com/raw/libMesh/libmesh/pull/3135.diff"
    SHA512 ...
    FILENAME libmesh-3135.diff
)
# ${PATCH1} is the location of the downloaded patch and can be passed to PATCHES in vcpkg_from_github()

(You can also get an individual commit: https://github.com/libMesh/libmesh/commit/ac5ebe88de3c15a51b9cf79ad5e8c5244beb6337.diff)

That helps us by ensuring (by construction) that upstream has everything for evaluation and that we aren't checking large amounts of code into the vcpkg registry.

@mkhon
Copy link
Contributor Author

mkhon commented Feb 8, 2022

@ras0219-msft libMesh/libmesh#3144 has been merged
Additionally, libMesh/libmesh#3147 and libMesh/libmesh#3148 have been merged to libmesh master as well, so the only remaining patches are contrib/ patches which will unlikely to be accepted (see libMesh/libmesh#3149), but they are minor and trivial.

Unfortunately, patches for merged PRs can not be taken from libmesh github as they are done against master (so we will need to update libmesh version first).

I would suggest to merge this PR as is, then when we will update libmesh version we can remove merged patches.

@ras0219-msft ras0219-msft added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Feb 9, 2022
@BillyONeal
Copy link
Member

Unfortunately, patches for merged PRs can not be taken from libmesh github as they are done against master (so we will need to update libmesh version first).

Personally I would prefer to do that update first. I tried to do so but ran into

configure: error: You must run "git submodule update --init --recursive" before configuring libmesh

I'm assuming @ras0219-msft tagged this because normally we would not accept patches of this magnitude, but the reason we normally won't accept them is because we are worried about updates and "owning" the consequences of those patches. Seeing that they are already merged upstream mitigates that concern. Even so the patches are huge and may even be scary for licensing reasons.

Hopefully we should have an answer for you on Thursday.

@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Feb 16, 2022
@BillyONeal
Copy link
Member

We discussed this "in person" today and agreed that we would like to see an update to the current shipping copy of libMesh before attempting to apply these patches, with the hope that that means the patches actually applied upstream will apply cleanly.

Because 1.6.2 has submodules that need to be fixed up, if you're interested in tackling that you'll need to call vcpkg_from_github multiple times and rename the resulting bits into place; for an example, check out what gstreamer is doing:

vcpkg_from_github(
OUT_SOURCE_PATH GST_PLUGIN_BASE_SOURCE_PATH
REPO gstreamer/gst-plugins-base
REF 1.19.2
SHA512 d2005e6a3bda5f08395b131347e8f4054c2469e04e65d1acc1a1572bf10d81d4dad4e43d6a8600346b6175a2310f81157a0cd27398ef69b5363b16346febfb39
HEAD_REF master
PATCHES ${PLUGIN_BASE_PATCHES}
)
vcpkg_from_github(
OUT_SOURCE_PATH GST_PLUGIN_GOOD_SOURCE_PATH
REPO gstreamer/gst-plugins-good
REF 1.19.2
SHA512 71e9f36d407db3b75d9a68f6447093aa011b2b586b06e0a1bb79c7db37c9114de505699e99a4dad06d8d9c742e91f48dd35457283babe440f88a9e40d3da465b
HEAD_REF master
PATCHES ${PLUGIN_GOOD_PATCHES}
)
vcpkg_from_github(
OUT_SOURCE_PATH GST_PLUGIN_BAD_SOURCE_PATH
REPO gstreamer/gst-plugins-bad
REF 1.19.2
SHA512 f63ca3abf380bba92dca4ac3a51cba5ea95093693cf64d167a7a9c0bf6341c35a74fd42332673dbd1581ea70da0a35026aa3e2ce99b5e573268ccb55b5491c1d
HEAD_REF master
)
vcpkg_from_github(
OUT_SOURCE_PATH GST_PLUGIN_UGLY_SOURCE_PATH
REPO gstreamer/gst-plugins-ugly
REF 1.19.2
SHA512 70dcd4a36d3bd35f680eaa3c980842fbb57f55f17d1453c6a95640709b1b33a263689bf54caa367154267d281e5474686fedaa980de24094de91886a57b6547a
HEAD_REF master
)
vcpkg_from_gitlab(
GITLAB_URL https://gitlab.freedesktop.org
OUT_SOURCE_PATH GST_MESON_PORTS_SOURCE_PATH
REPO gstreamer/meson-ports/gl-headers
REF 5c8c7c0d3ca1f0b783272dac0b95e09414e49bc8 # master commit. Date 2021-04-21
SHA512 d001535e1c1b5bb515ac96c7d15b25ca51460a5af0c858df53b11c7bae87c4a494e4a1b1b9c3c41a5989001db083645dde2054b82acbbeab7f9939308b676f9c
HEAD_REF master
)
file(RENAME ${GST_SOURCE_PATH} ${GST_BUILD_SOURCE_PATH}/subprojects/gstreamer)
file(RENAME ${GST_PLUGIN_BASE_SOURCE_PATH} ${GST_BUILD_SOURCE_PATH}/subprojects/gst-plugins-base)
file(RENAME ${GST_PLUGIN_GOOD_SOURCE_PATH} ${GST_BUILD_SOURCE_PATH}/subprojects/gst-plugins-good)
file(RENAME ${GST_PLUGIN_BAD_SOURCE_PATH} ${GST_BUILD_SOURCE_PATH}/subprojects/gst-plugins-bad)
file(RENAME ${GST_PLUGIN_UGLY_SOURCE_PATH} ${GST_BUILD_SOURCE_PATH}/subprojects/gst-plugins-ugly)
file(RENAME ${GST_MESON_PORTS_SOURCE_PATH} ${GST_BUILD_SOURCE_PATH}/subprojects/gl-headers)

@BillyONeal BillyONeal added requires:author-response and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Feb 17, 2022
@BillyONeal
Copy link
Member

@ras0219-msft also indicates that the release tarball (which you'd get with vcpkg_download_distfile) may already have the right submoduled content included.

@JackBoosY
Copy link
Contributor

Ping for response.

@mkhon
Copy link
Contributor Author

mkhon commented May 7, 2022

There is an issue with linking libmesh after I updated to the latest libmesh (with Windows build fixes)

@JackBoosY
Copy link
Contributor

@mkhon Can you please provide the failure logs?

@PhoebeHui PhoebeHui marked this pull request as draft June 9, 2022 06:31
@JackBoosY
Copy link
Contributor

Ping for response again.

@JackBoosY
Copy link
Contributor

Closing this PR since it seems that no progress is being made. Please reopen if work is still being done.

@JackBoosY JackBoosY closed this Aug 19, 2022
@mkhon mkhon deleted the fix/libmesh-windows-build branch September 28, 2022 19:11
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants