libMesh / libmesh

libMesh github repository
http://libmesh.github.io
GNU Lesser General Public License v2.1
659 stars 286 forks source link

Add Netgen to distributed tarballs #4003

Closed roystgnr closed 4 days ago

roystgnr commented 2 weeks ago

We seem to have been passing make distcheck only because configure gracefully disabled Netgen for us?

I'm still testing builds of tarballs-now-with-netgen myself, but I think it's working.

moosebuild commented 2 weeks ago

Job Coverage, step Generate coverage on a2327e5 wanted to post the following:

Coverage

d9fe3d #4003 a2327e
Total Total +/- New
Rate 62.33% 62.33% -0.00% -
Hits 72649 72648 -1 0
Misses 43900 43901 +1 0

Diff coverage report

Full coverage report

This comment will be updated on new commits.

jwpeterson commented 2 weeks ago

Once this is merged to master, it will also need to be cherry-picked onto branch_v1.8.0. It was while testing make dist on the release branch that we first realized the netgen sources were not in the distribution tarball. Once that problem was fixed, further complications arose with actually building netgen from the release tarball.

moosebuild commented 2 weeks ago

Job Test mac on c339dcf : invalidated by @jwpeterson

Invalidate to see if PackagesNotFoundError for moose-petsc goes away

roystgnr commented 1 week ago

Wait, did we just pass the distcheck test in CI? I swear I'm still having some issues locally.

jwpeterson commented 1 week ago

Difficult to say what happened exactly. The output of the DistCheck step here exceeded the output size limit:

*****************************************************

CIVET: Output size exceeded limit (5242880 bytes), further output will not be displayed!

*****************************************************
jwpeterson commented 1 week ago

Looks like Netgen was configured and built during the DistCheck step:

  Netgen :  Automatic configuration OK.

...

[ 99%] Building CXX object CMakeFiles/nglib.dir/nglib/nglib.cpp.o
[100%] Linking CXX shared library libnglib.so

It looks like it was in the process of deleting everything when the output size limit was exceeded, so it kind of seems like it worked.

Making uninstall in examples
jwpeterson commented 1 week ago

@roystgnr what issues are you still seeing locally with this branch? I pulled this branch and did the following:

  1. make dist
  2. Untar tarball in fresh directory.
  3. configure, ensure that netgen was enabled
  4. make install
  5. make check
  6. Go to unit test directory and run: ./unit_tests-opt --re testNet

The last step produced the following output:

Will run the following tests:
MeshTetTest::testNetGen
MeshTetTest::testNetGenError
MeshTetTest::testNetGenTets
MeshTetTest::testNetGenFlippedTris
MeshTetTest::testNetGenHole
MeshTetTest::testNetGenSphereShell
--- Running 6 tests in total.
......

OK (6 tests)

So, as far as I can tell, this fixes all the issues that we had before with make dist and netgen.

I think this is the last thing (or one of the last things) I'm going to cherry-pick onto branch_v1.8.0 before tagging 1.8.0-rc1 so let me know ASAP if there is still some issue to be resolved here.

jwpeterson commented 1 week ago

@roystgnr I tested your $DESTDIR update and it still worked fine for the sequence of tests I mentioned in this comment, so if it also fixes the distcheck errors you were seeing locally, I think we should be good to merge. (I've already approved this PR, but if I could approve it again, I would :face_holding_back_tears:)

roystgnr commented 5 days ago

I wish we were good to merge. The $DESTDIR fix gets my build working fine, but then the subsequent distclean fails because I didn't delete my build directory. I'm actually not seeing anything outside the build directory (the only failure there was fixed in our netgen branch and not general to cmake) so this ought to be an easy fix too.

roystgnr commented 4 days ago

Okay, now I think we're ready to merge once CI is done again.

jwpeterson commented 4 days ago

I confirmed that make distclean also works when building from the tarball. I will go ahead and merge when the testing is done, unless you get to it first.