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

contrib: MS VC build fixes #3149

Open
wants to merge 2 commits into
base: devel
Choose a base branch
from

Conversation

mkhon
Copy link
Contributor

@mkhon mkhon commented Jan 30, 2022

No description provided.

@mkhon mkhon force-pushed the fix/contrib-msvc-build branch 2 times, most recently from 851dc44 to cbfd3c7 Compare January 30, 2022 15:37
@moosebuild
Copy link

moosebuild commented Jan 30, 2022

Job Coverage on 0175262 wanted to post the following:

Coverage

Coverage did not change

Full coverage report

This comment will be updated on new commits.

@mkhon mkhon force-pushed the fix/contrib-msvc-build branch from cbfd3c7 to 5977f49 Compare January 30, 2022 16:42
@dschwen
Copy link
Member

dschwen commented Jan 30, 2022

Max, some updated build instructions for msys2/VC (including PETSc) would be really useful!

@roystgnr
Copy link
Member

It's not nearly as bad as the "will be reverted the next time someone runs bootstrap" changeset, but this still seems to be asking for regressions.

Making these sorts of changes in libMesh proper, maybe a little tricky to maintain, but at least we maintain it. We don't maintain Exodus, we just download and update when we need a newer version. Likewise for NetCDF. And for Metis, it's even worse - we don't even always use our own contrib/metis files; we'll build against a third party's (most commonly PETSc's) as necessary. So half your changes here are going to be reverted the next time we update a contrib package (or in one case might have already been reverted - are the Exodus changes here 5.22-only because 8.11 worked perfectly, or just because you didn't test with 8.11?), and half of the changes will occasionally go unused.

I'm afraid there's nothing to be done about the "built against a third party's" case, except to submit the same changes upstream and hope that third parties sync up before it becomes a problem. Have you done that yet?

But if our "download and update" isn't to become "download and update and break your builds without realizing it", we really need some more documentation here. A set of contrib/patches/*.diff files that can be examined (or if we're lucky, just fed into patch maybe?) whenever we upgrade? That might be considered redundant with the git logs ... but we ought to at least have some contrib/README pointing out how and why an upgrade might be more complicated than expected. See contrib/boost/README for a good example.

And updated build instructions would very much help in the same regard. If we're at the point where the remaining fixes are in contrib dependencies, and something like a --disable-exodus --disable-netcdf --disable-metis build might be just about working in Windows, let us know how, so we can replicate it and test against it when making changes we expect to impact it?

@mkhon
Copy link
Contributor Author

mkhon commented Feb 4, 2022

In the meantime you can see the configure arguments I used here: microsoft/vcpkg#22810
This is msys2 + configure + MS VC build.

vcpkg sets up its own environment and also vcpkg libmesh port does some custom commands in its portfile.cmake so non-vcpkg build will be rather complicated.

I will submit a separate PR with non-vcpkg build instructions later.

Additionally, we can keep contrib bits as is (the patch will be maintained in vcpkg repository) and this PR can be closed.

@mkhon mkhon changed the title MS VC build fixes contrib: MS VC build fixes Feb 4, 2022
@jwpeterson
Copy link
Member

This PR has been marked "do not merge" since we are no longer accepting PRs into the master branch. All new PRs should be made on the devel branch instead. Once this PR's target branch has been updated to devel, the "do not merge" label will be removed.

@mkhon mkhon changed the base branch from master to devel May 5, 2022 14:32
@mkhon
Copy link
Contributor Author

mkhon commented May 5, 2022

This PR has been marked "do not merge" since we are no longer accepting PRs into the master branch. All new PRs should be made on the devel branch instead. Once this PR's target branch has been updated to devel, the "do not merge" label will be removed.

target branch is now devel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants