Vsp3.34.0 Build Modifications #269
Conversation
Two variables appear to be missing from Python API and have been commented out. If test_outputs directory does not exist, the exception will be thrown. Adding. t.setupcls() forces directory to be created if one does not exist.
|
I commented on these commits directly (before you made this pull request). I'm not sure how to access those comments from here, I will copy/paste them in (with some more added comments).... Change 1: The preferred way to build on Ubuntu is to use the system installed version of LibXML2. I.e. this (LibXML2) should never be built. https://github.com/OpenVSP/OpenVSP/blob/main/.github/workflows/build.yml#L77 Although Ubuntu prefers to install libraries like this built as shared libs, OpenVSP as a policy avoids shared libs when we build libraries ourselves. You can't change something like a build option to a library without testing it on all three platforms. I promise that this change will break Windows and MacOS builds. Change 2: We should either 1) modify the build system to auto-generate this part of test.py such that all enums are included in the test (sounds unrealistic). Or 2) reduce this portion of the test down to a few key enums to prove that enums in general are coming through. Change 3: No comment. |
|
Not sure where the comments went. I didn't get a notification, and I can't
seem to find them.
Per 1: Understood. Irian has been making an effort to use no
system-dependent libraries such that he can provide a consistent set of
build scripts across platforms at different centers. At least, that was
the idea. FWIW, I tested the whole build process on both Linux and Windows
by removing the --disabled-shared option and they both built and tested
without issues. So, I did consider the platform testing, at least for the
platforms I have access to. I did all that verification prior to the PR.
What I don't understand is why the option causes it to throw an error? Why
can't both work?
Per 2: Makes sense, and I recall our discussion of trying to figure out
Python testing a while back. Until your message, I had forgotten about all
of this. Any reason not to delete the ones that don't work so that test.py
works, even if it is not comprehensive? Or, do you think that provides a
false sense of success? Or, does it not mean anything? I use it to know
that the OpenVSP Python API is actually working, nothing more. Running
t.setUpClass() is still required to ensure a directory is made.
…On Tue, Jun 27, 2023 at 1:30 AM Rob McDonald ***@***.***> wrote:
I commented on these commits directly (before you made this pull request).
I'm not sure how to access those comments from here, I will copy/paste them
in (with some more added comments)....
Change 1:
The preferred way to build on Ubuntu is to use the system installed
version of LibXML2. I.e. this (LibXML2) should never be built.
https://github.com/OpenVSP/OpenVSP/blob/main/.github/workflows/build.yml#L77
Although Ubuntu prefers to install libraries like this built as shared
libs, OpenVSP as a policy avoids shared libs when we build libraries
ourselves.
You can't change something like a build option to a library without
testing it on all three platforms. I promise that this change will break
Windows and MacOS builds.
Change 2:
This test is hugely fragile. The list of enums is hard-coded from whenever
this test was created (long ago and far away). Since removed enums like
these throw errors, but there are tons of newly added enums that are not
checked -- with no errors issued.
We should either 1) modify the build system to auto-generate this part of
test.py such that all enums are included in the test (sounds unrealistic).
Or 2) reduce this portion of the test down to a few key enums to prove that
enums in general are coming through.
Change 3: No comment.
—
Reply to this email directly, view it on GitHub
<#269 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIBJX2UBFCAZTOD2LH77QFTXNJVVTANCNFSM6AAAAAAZUHQ6EQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
When you tested the changed build system on Windows, did you delete the CMake Cache (i.e. start over clean?). Sometimes some of these settings can be difficult to 'take root'. If the flag works the way it should, it should have forced all platforms to build shared libraries for LibXML2 -- which then means that it would not be statically linked into the executable. Nothing should change on Linux (because it already uses shared libraries when you use the system installed lib) On Windows, you should notice this because 1) the executable gets smaller (LibXML2 is now dynamic (outside) rather than static (inside the executable)) and 2) it won't run on a computer that doesn't have the LibXML2 dynamic library in a place it can be found (it may already be on many computers, so this might in fact be a difficult failure to detect). BTW, is Irian distributing the LibXML2 *.so dynamic library to the computers that he is targeting? If he isn't statically linking -- and he isn't counting on the system-installed library -- then he needs to be distributing the library himself. LibXML2 is so common in the Linux world that I suspect all the computers he is targeting already have it installed and this exercise is a big waste of time. What error does having the option set cause? I believe we're shipping a fairly old LibXML2 library and it is likely that the static lib build is simply broken on Linux because nobody ever uses it. Re 2) I would happily accept a patch that deleted most of the enums from the test.py. I think it makes sense to leave a handful of core ones that won't ever go away -- just to test that SWIG and everything are succeeding in creating them and passing them through. However, I don't think we should attempt to have an exhaustive list there unless we figure out a way to auto-generate it. If you need some simple test to check that the API is available and working, I recommend you write one. |
Build on Ubuntu 22 fails with LibXML error. Found that removing --disable-shared form *LibXml2.cmake resolves this issue. Also discovered and resolved in same way by Irian O. When testing, need to execute setUpClass() to ensure test_outputs document is generated if it does not exist, and resolved API testing error for two variables that do not exist.
Pip dependency was added for environment setup as setup process will fail on a clean, out-of-the-box system that has never used pip previously.