Skip to content

Conversation

@mkhon
Copy link
Contributor

@mkhon mkhon commented Jan 27, 2022

libtool "export all" linking does not pick them up due to their sections marked as "(pick any)" in dumpbin output

@mkhon
Copy link
Contributor Author

mkhon commented Jan 27, 2022

This patch is a part of changes to build libmesh with libtool (msys2) and MS VC on Windows.
See microsoft/vcpkg#22810

@moosebuild
Copy link

moosebuild commented Jan 27, 2022

Job Coverage on 10dae09 wanted to post the following:

Coverage

Coverage did not change

Full coverage report

This comment will be updated on new commits.


#define FACE_EDGE_SHAPE_ERROR(_dim, _func) \
template <> \
template <> \
Copy link
Member

Choose a reason for hiding this comment

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

No, thanks, the whitespace looks better aligned...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a left-over from my previous attempt where I also modified template specializations (which turned out to be not required). Corrected in the updated PR

@roystgnr
Copy link
Member

I guess this isn't quite as maintenance-fragile as it looks, because if we forget to put that declspec on an instantiation, that only impacts the ability to call the instantiated function from code external to the DLL, not internally? So e.g. you don't have it in the LIBMESH_DEFAULT_VECTORIZED_FE specializations in fe.h, but you didn't notice because we never call those methods from examples, only from inside other library functions?

I'm fine with this, but it still seems like it'll regress in no time flat unless we can figure out how to get it in CI.

@jwpeterson
Copy link
Member

but it still seems like it'll regress in no time flat

Agreed, unless there happens to be another LIBMESH_EXPORT nearby when I'm writing code, it's likely I'll forget to include it.

I glanced briefly at your other PR:

speed up configure stage 3x (from 1.5h to 0.5h on my build box) by optimizing .Tpo file creation

This seems very painful. Is this an existing/known issue with the autotools on Windows, or is there something specific to our configure scripts that makes them so slow?

@roystgnr
Copy link
Member

unless there happens to be another LIBMESH_EXPORT nearby when I'm writing code, it's likely I'll forget to include it.

And ironically, the better we do when writing new code, the worse the omission is, I think? If we slack off and don't add any test coverage, then DLL-libMesh doesn't have the new API exported, but at least existing builds and user codes will be fine, they just won't be able to update to use the new API right away. If we're conscientious and add a unit test or example invoking the new API, then the test or example won't link and we've broken make check on Windows.

I think we can probably support this on a "make sure it works before each new release" basis, though, even if it breaks from git hash to git hash. Looks like Visual Studio Community is free for individuals and for open source communities to use?

@mkhon
Copy link
Contributor Author

mkhon commented Jan 29, 2022

I guess this isn't quite as maintenance-fragile as it looks, because if we forget to put that declspec on an instantiation, that only impacts the ability to call the instantiated function from code external to the DLL, not internally?

Right

So e.g. you don't have it in the LIBMESH_DEFAULT_VECTORIZED_FE specializations in fe.h, but you didn't notice because we never call those methods from examples, only from inside other library functions?

No. I tried to find all explicit template instantiations and mark them with LIBMESH_EXPORT.

LIBMESH_DEFAULT_VECTORIZED_FE is not an explicit template instantiation - it is a full template specialization and MS VC 2019 + libtool do not have issue with that - MS VC puts these into the section marked as "(pick no duplicates)" and libtool automatic "export all" implementation picks the up for the export

I created a simple test demonstrating this:
https://github.com/soylent-io/vcpkg/tree/libtool-template-instantiation/ports/libtool-template-instantiation
https://github.com/mkhon/libtool-template-instantiation/blob/master/test.cpp

I'm fine with this, but it still seems like it'll regress in no time flat unless we can figure out how to get it in CI.

It is enough to have a good coverage in unit tests (current tests provide a good basis for this).

@mkhon
Copy link
Contributor Author

mkhon commented Jan 29, 2022

speed up configure stage 3x (from 1.5h to 0.5h on my build box) by optimizing .Tpo file creation

This seems very painful. Is this an existing/known issue with the autotools on Windows, or is there something specific to our configure scripts that makes them so slow?

This is related to how automake generates .Plo dep files: currently it executes a shell command for each .Plo file (just writing "# dummy" there) at "executing depfiles commands" stage of configure script (doing "make am--depfiles" for each Makefile).

Process execution is very expensive in Windows so this stage (which takes several seconds on Linux) executes for hours
on Windows.

I don't think we should do anything in libmesh repo about this.

@mkhon
Copy link
Contributor Author

mkhon commented Jan 29, 2022

Looks like Visual Studio Community is free for individuals and for open source communities to use?

yes

libtool "export all" linking does not pick them up due to their sections
marked as "(pick any)" in dumpbin output
@mkhon mkhon force-pushed the feature/libmesh-export branch from 7ff07fc to 10dae09 Compare January 29, 2022 07:05
@roystgnr
Copy link
Member

It is enough to have a good coverage in unit tests (current tests provide a good basis for this).

Sure, but a test is only a test when it's run, and we don't have build boxes available to run regular tests on Windows, and I fear we may never. So I'll bet we see this regress regularly in the git master, only getting fixed when we run extra tests manually before making an official release.

Process execution is very expensive in Windows so this stage (which takes several seconds on Linux) executes for hours
on Windows.

That's dumbfounding. I knew process creation has long been much more expensive than thread creation in Windows, but so expensive that it dominates I/O even, and by orders of magnitude?

@roystgnr roystgnr merged commit 0d1c0e5 into libMesh:master Feb 8, 2022
@mkhon mkhon deleted the feature/libmesh-export branch February 8, 2022 21:38
@mkhon
Copy link
Contributor Author

mkhon commented Feb 21, 2022

@jwpeterson @roystgnr can we have a tag and a pre-release tarball for this? vcpkg team asked for a port that builds from pristine sources with all the patches applied

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.

4 participants