starseeker (May 20 2020 at 23:53): starseeker (May 20 2020 at 23:53):Ouch. This is going to be a job - my using CPP_DLL_DEFINES instead of MSVC to denote a Visual Studio compile is going to boomerang big time. Will actually have to go back and put the proper MSVC symbols in.
Also, at first look gcc doesn't really support the Windows syntax:
include/brlcad/bu/defines.h:43:33: error: expected constructor, destructor, or type conversion before ‘(’ token
43 | # define BU_EXPORT __declspec(dllimport)
| ^
include/brlcad/bu/app.h:54:1: note: in expansion of macro ‘BU_EXPORT’
54 | BU_EXPORT extern const char *bu_argv0_full_path(void);
| ^~~~~~~~~
This example makes me think they're not a 1-1 swap:
// Generic helper definitions for shared library support
#if defined _WIN32 || defined __CYGWIN__
#define FOX_HELPER_DLL_IMPORT __declspec(dllimport)
#define FOX_HELPER_DLL_EXPORT __declspec(dllexport)
#define FOX_HELPER_DLL_LOCAL
#else
#if __GNUC__ >= 4
#define FOX_HELPER_DLL_IMPORT __attribute__ ((visibility ("default")))
#define FOX_HELPER_DLL_EXPORT __attribute__ ((visibility ("default")))
#define FOX_HELPER_DLL_LOCAL __attribute__ ((visibility ("hidden")))
#else
#define FOX_HELPER_DLL_IMPORT
#define FOX_HELPER_DLL_EXPORT
#define FOX_HELPER_DLL_LOCAL
#endif
#endif
The BU_EXPORT/BU_IMPORT definitions didn't seem to work - the only thing that actually succeeded was putting __attribute__ ((visibility ("default"))) before the actual implementations in the .c files. I must be missing something...
starseeker (May 21 2020 at 00:20):Ah - the definitions aren't making it to the command line. Why....
starseeker (May 21 2020 at 00:23):Bingo. the ${libname}-obj target needs it as well.
starseeker (May 21 2020 at 00:27):Based on that GCC page, we're looking at something like:
#ifndef BU_EXPORT
# if defined(BU_DLL_EXPORTS) && defined(BU_DLL_IMPORTS)
# error "Only BU_DLL_EXPORTS or BU_DLL_IMPORTS can be defined, not both."
# elif defined(BU_DLL_EXPORTS)
# if defined(_WIN32)
# define BU_EXPORT __declspec(dllexport)
# else
# define BU_EXPORT __attribute__ ((visibility ("default")))
# endif
# elif defined(BU_DLL_IMPORTS)
# if defined(_WIN32)
# define BU_EXPORT __declspec(dllimport)
# else
# define BU_EXPORT __attribute__ ((visibility ("default")))
# endif
# else
# define BU_EXPORT
# endif
#endif
@Sean Assuming it doesn't break any rules, my vote would be to do something like this once in common.h:
#if defined(_WIN32)
# define DLLEXPORT __declspec(dllexport)
# define DLLIMPORT __declspec(dllimport)
#else
# define DLLEXPORT __attribute__ ((visibility ("default")))
# define DLLIMPORT __attribute__ ((visibility ("default")))
#endif
And then rework our existing defines.h headers to do:
#ifndef BU_EXPORT
# if defined(BU_DLL_EXPORTS) && defined(BU_DLL_IMPORTS)
# error "Only BU_DLL_EXPORTS or BU_DLL_IMPORTS can be defined, not both."
# elif defined(BU_DLL_EXPORTS)
# define BU_EXPORT DLLEXPORT
# elif defined(BU_DLL_IMPORTS)
# define BU_EXPORT DLLIMPORT
# else
# define BU_EXPORT
# endif
#endif
The bioh branch r75860 has a build that works with Ubuntu gcc 9.3.0 -fvisibility=hidden. I've not tested it extensively, and not tried it with clang at all, but it should be a reasonable approximation of what will be needed to make this work...
Sean (May 21 2020 at 04:33):starseeker said:
Ouch. This is going to be a job - my using CPP_DLL_DEFINES instead of MSVC to denote a Visual Studio compile is going to boomerang big time. Will actually have to go back and put the proper MSVC symbols in.
Yuck, using MSVC or _WIN32 is never proper... Should be keyed on a feature test from the build system, like a HAVE_DECLSPEC test. Even better if conditionals can be eliminated so it all turns into a build provision, otherwise it's usually best to leave things obvious and direct.
Also, at first look gcc doesn't really support the Windows syntax:
From the comment, must have only been gcc on windows then.
Sean (May 21 2020 at 04:41):starseeker said:
Sean Assuming it doesn't break any rules, my vote would be to do something like this once in common.h:
Doesn't break any rules, and common.h mod looks good except for the usage of _WIN32.
And then rework our existing defines.h headers to do:
#ifndef BU_EXPORT # if defined(BU_DLL_EXPORTS) && defined(BU_DLL_IMPORTS) # error "Only BU_DLL_EXPORTS or BU_DLL_IMPORTS can be defined, not both." # elif defined(BU_DLL_EXPORTS) # define BU_EXPORT DLLEXPORT # elif defined(BU_DLL_IMPORTS) # define BU_EXPORT DLLIMPORT # else # define BU_EXPORT # endif #endif
This is an improvement! If you can get an export/import define written out into the config header after testing for it, you won't need anything in common.h too.
starseeker (May 21 2020 at 15:28):@Sean OK, I've gotten it doing a configure test and using brlcad_config.h instead of common.h. It's probably worth testing the bioh branch on the Mac now to see how it reacts. I'm testing Windows (which for once is the platform where nothing should change if we're good) and if everything passes I'd like to merge back to trunk.
Sean (May 22 2020 at 02:37):@starseeker awesome, okay will test it on mac tonight.
Sean (May 23 2020 at 06:24):mged is crashing inside libdm initialization:
Call location:
2020-05-23 02:20:18.432190-0400 mged[11937:31932533] 0 CarbonCore 0x00007fff40f58fee ___Gestalt_SystemVersion_block_invoke + 112
2020-05-23 02:20:18.432202-0400 mged[11937:31932533] 1 libdispatch.dylib 0x00007fff6bb8263d _dispatch_client_callout + 8
2020-05-23 02:20:18.432212-0400 mged[11937:31932533] 2 libdispatch.dylib 0x00007fff6bb83d4b _dispatch_once_callout + 20
2020-05-23 02:20:18.432222-0400 mged[11937:31932533] 3 CarbonCore 0x00007fff40efa992 _Gestalt_SystemVersion + 945
2020-05-23 02:20:18.432231-0400 mged[11937:31932533] 4 CarbonCore 0x00007fff40ef3e7c Gestalt + 149
2020-05-23 02:20:18.432237-0400 mged[11937:31932533] 5 Tk 0x00007fff4ba9ca53 TkpOpenDisplay + 573
2020-05-23 02:20:18.432247-0400 mged[11937:31932533] 6 Tk 0x00007fff4ba07d33 TkCreateMainWindow + 1178
Process 11937 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x28)
frame #0: 0x0000000102ea1b48 libX11.6.dylib`XGetVisualInfo + 299
libX11.6.dylib`XGetVisualInfo:
-> 0x102ea1b48 <+299>: cmpl $0x0, 0x28(%rax,%rcx)
0x102ea1b4d <+304>: jle 0x102ea1d2d ; <+784>
0x102ea1b53 <+310>: leaq 0x30(%rax,%rcx), %rdx
0x102ea1b58 <+315>: movq %rdx, -0x88(%rbp)
Target 0: (mged) stopped.
(lldb) up
frame #1: 0x00000001007024de libdm.20.dylib`ogl_choose_visual(dmp=0x0000000105a21e40, tkwin=0x00000001060f6010) at dm-ogl.c:496:14
493 /* Try to satisfy the above desires with a color visual of the
494 * greatest depth */
495
-> 496 vibase = XGetVisualInfo(((struct dm_xvars *)dmp->dm_vars.pub_vars)->dpy,
497 0, &vitemp, &num);
here's the relevant backtrace:
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x28)
frame #0: 0x0000000102ea1b48 libX11.6.dylib`XGetVisualInfo + 299
* frame #1: 0x00000001007024de libdm.20.dylib`ogl_choose_visual(dmp=0x0000000105a21e40, tkwin=0x00000001060f6010) at dm-ogl.c:496:14
frame #2: 0x0000000100701c59 libdm.20.dylib`ogl_open(interp=0x0000000107010a10, argc=6, argv=0x0000000107015b78) at dm-ogl.c:859:23
frame #3: 0x0000000100711428 libdm.20.dylib`dm_open(interp=0x0000000107010a10, type=4, argc=7, argv=0x0000000107015b70) at dm-generic.c:131:13
frame #4: 0x000000010001218c mged`mged_dm_init(o_dm_list=0x0000000106002200, dm_type=4, argc=8, argv=0x0000000107015b70) at attach.c:273:16
frame #5: 0x000000010001392c mged`mged_attach(wp=0x00000001000fc908, argc=8, argv=0x0000000107015b70) at attach.c:655:9
frame #6: 0x00000001000133a7 mged`f_attach(UNUSED_clientData=0x0000000100103228, interpreter=0x0000000107010a10, argc=8, argv=0x0000000107015b70) at attach.c:519:12
I don't know where it's coming from, but this is feeling like a compilation mismatch, because it detected and successfully compiled against the system Tcl/Tk 8.5
I'm guessing, but could be the display from Tk_Display() getting set to dpy in dm_open is a non-X11 tk "display" and we later assume it's an x11 display.
It wasn't previously using system Tk, so presumably either some check was removed/relaxed that prevented it or something was addd/enabled that turned it on.
Sean (May 23 2020 at 06:36):Any ideas?
Sean (May 23 2020 at 07:42):so an enable all build seems to confirm it, so just something untintended pulled in or let out that is letting system Tk try to run despite not enabling an aqua build. That being the case, it should be okay to merge as trunk doesn't seem to have the issue, and if it does, then it's not unique to the branch. Will need to get fixed but shoul be good to merge.
starseeker (May 23 2020 at 13:51):OK, thanks - merged. You say it's getting a system Tcl/Tk 8.5? That should have failed the version number test...
The most likely thing I can think of offhand is that I didn't translate some portion of the X11 vs Aqua testing logic when I made the 8.5 -> 8.6 transition, and the new FindTCL logic is succeeding incorrectly. I switched from our custom, messy pile of Tcl detection logic to a combination of the standard CMake Find modules so it's actually quite likely FindTCL is now working "better" than it used to, although that should have manifested not long after the 8.6 merge went in...
What is actually needed is an additional detection that will fail an otherwise successful FindTCL based on the windowing system used by Tk. As far as I can tell there's no way to have Tcl/Tk tell you what windowing system is in use without actually initializing a Tk window, which breaks the build on headless machines. If there is a way to detect the Tk windowing system during configure, that's almost certainly what we need...
starseeker (May 23 2020 at 13:53):Interesting that's it's trying an OGL attach - I wonder if it found an X11 OpenGL and the Aqua Tcl/Tk?
starseeker (May 23 2020 at 13:54):Trying to keep all that straight on the Mac was one of the reasons (the main one, really) our custom FindTCL.cmake was so complicated.
Sean (May 23 2020 at 15:27):I'll have to check more closely, but worth noting that I don't think it's getting system Tk on trunk, so presumably you just pulled too little or too much on the branch.
Sean (May 23 2020 at 15:27):at least, mged hasn't segfaulted on me there yet
starseeker (May 23 2020 at 15:30):Could be - those merges can be a real pain in the neck...
Sean (May 23 2020 at 15:42):worth mentioning that "make regress" took 18 minutes on my laptop. I don't know if I just had it particularly busy at the time, but all the tests ran slower than usual for some reason.
starseeker (May 23 2020 at 17:06):That's.. peculiar. I pushed a few changes back from trunk to bioh, but none of them should have caused either that or the Tcl/Tk searching change.
I pushed a test into bioh trying to get a read on the Tk windowing system - I can't properly test it here, but I'd be curious to know what it does with the mac system Tk if you can run a configure (it's a configure time test, so will succeed/fail right away.)
Sean (May 23 2020 at 17:17):I can run it, but not digging the approach nor understand the motivation. That's a heck of a lot of complexity and code that could go wrong to detect an issue on one platform that I'd said I hadn't seen on trunk...
Sean (May 23 2020 at 17:21):If we needed a test (and it's not clear that we do), the feature test would be to simply link against Tk, call Tk_Init+Tk_Display followed by an X11 call like XNextEvent or something simple.
starseeker (May 23 2020 at 18:05):@Sean It's not specifically for the issue you indicated you saw - it's an attempt to solve more robustly something I've tried to solve several times over the years, always unsuccessfully - a configure time test to identify from the Tcl/Tk installation we have detected what graphics system it uses.
Calling Tk_Init, if I'm understanding the libtk code correctly, is enough all by itself to cause the graphics system test to fail on something like a headless Jenkins build client. It tries to create a graphics window, fails, and the test fails. So trying to specify Aqua vs X11 is iffy, because we don't know if we've got the right Tcl/Tk at configure time.
The correct long term fix is to get our code to the point where we don't care if it's an Aqua, Win32 or X11 build of Tk, but we're quite a ways from that. (Actually, I'm not sure we can ever get there until Tcl/Tk provides its own built-in OpenGL widget.)
Sean (May 23 2020 at 18:29):That's a want, not a problem... what's the problem being fixed? Every conceivable problem I can think of has a far simpler solution that should be more than adequate for whatever short remaining time we have left with Tk.
starseeker (May 23 2020 at 18:31):OK. The "problem" as I saw it was the risk of a Mac user doing a configure + build and getting a cryptic failure because the configure process mixed and matched incorrectly. That's the motivation, so if the cost of resolving it is too high I'm good with skipping it.
Sean (May 23 2020 at 18:32):We shouldn't be caring whether we're win32 or x11 as those are platform distinctions, so the only issue is aqua vs non-aqua.
starseeker (May 23 2020 at 18:33):As long as we're not trying to build X11 on Windows, that's correct
Sean (May 23 2020 at 18:34):Those are details that could just as easily be a requirement line in the respective README.* file (X11 on windows? you're on your own)
Sean (May 23 2020 at 18:34):(aquatk on mac? patches welcome, but you're on your own)
starseeker (May 23 2020 at 18:35):OK, works for me. I'll back it out.
Sean (May 23 2020 at 18:35):whatever we are/were doing on Mac has been working. it's not been until the 8.6 build that the build has pulled in a system tk on mac
Sean (May 23 2020 at 18:35):i'm not sure what was going on in the branch, but like I said, I've not (yet) seen it do that on trunk
starseeker (May 23 2020 at 18:36):Which is worrisome, because I didn't see anything in the branch that would have justified a different configure response
Sean (May 23 2020 at 18:36):haven't looked at the cmake logic to remind myself what criteria it's trying to use to ensure aquatk is pulled in without the cmake flag
Sean (May 23 2020 at 18:36):it's more likely something that was missing
starseeker (May 23 2020 at 18:37):I don't know what version OSX's native Tcl/Tk reports, but if it's 8.6 then for a long time version checking by itself would have avoided system detection - i.e. we may have been "accidentally" working
Sean (May 23 2020 at 18:37):It's 8.5
Sean (May 23 2020 at 18:38):we put something in the logic, I just don't recall what
Sean (May 23 2020 at 18:39):It wasn't accident, but it might have been tied to some related system feature like the availability of the Tk.framework or Carbon or something similar.
Sean (May 23 2020 at 18:40):I'm actually impressed that the branch got as far as it did. It would have been a good environment to try and make it work with dm-tk.
Sean (May 23 2020 at 18:42):technically what dm-ogl is doing is wrong -- it's assuming typeof(Tk_Display) == typeof(Display), compatible by cast, which is quite wrong. that's working by coincidence.
starseeker (May 23 2020 at 18:53):The old graphics platform detection is at https://sourceforge.net/p/brlcad/code/HEAD/tree/brlcad/tags/rel-7-30-8/misc/CMake/FindBRLCADTCL.cmake around line 273. called in turn at 750 by VALIDATE_TK, which in turn was called by the main "validate this Tcl/Tk installation" loops. That whole Find setup is a complex, messy implementation that tries too hard to resolve too much, which is why I fell back to the vanilla CMake standard modules for 8.6.
What may have happened in all that though, is whenever the graphics script failed (e.g. headless mode), we wound up not using system Tcl/Tk under any conditions even when it was otherwise present.) That wouldn't work for proper distcheck testing, but would "function" as long as the embedded build worked under any conditions.
starseeker (May 23 2020 at 18:55):Although in fairness, even the distcheck tests never pre-install the deps to make sure a SYSTEM configuration would function... Probably something to eventually set up
Sean (May 23 2020 at 18:59):You may be right as i'm not seeing any toggles any more currently in the code that will make it not pick system tk on mac, though the FindTCL logic isn't quite what I'd remember there being.
Sean (May 23 2020 at 19:00):that begs a question how trunk is working, I'll take a look (maybe it's not...)
starseeker (May 23 2020 at 19:04):If I need to I can reproduce that behavior with the new setup - i.e. fail a detection if either a) the windowing system test succeeds and the results are wrong for the selected config or b) the test simply fails to execute - and in either case fall back to enabling the build. I'd have to incorporate the old graphics check into the new FindTCL.cmake, but that shouldn't be too bad.
Sean (May 23 2020 at 19:05):you're right that it would be nice if whatever we get working can be made to work with the standard Findtcl
starseeker (May 23 2020 at 19:07):Well, it's a concatenation of several of their modules - they broke out tclsh, tcl, and a couple others. It's not really right - modern CMake will produce warnings about naming incompatibiltiies using them if one just uses the built in modules. I may try to submit the merged version back upstream.
starseeker (May 23 2020 at 19:09):I'll extract the old graphics check and see if I can incorporate it into the new find logic - it will be simpler in the sense that it will be a single pass/fail rather than looping through all found Tcl/Tk setups trying to find one that matches, but I think the post CMake 3.12 -D<PackageName>_ROOT=/path find_package behavior is a better answer to that.
starseeker (May 23 2020 at 19:10):And if we ever do decide we want a CI test that incorporates a system installed Tcl/Tk, it will break - we'll need to revisit it then.
Sean (May 23 2020 at 19:51):Could let standard find do it's job, then run our own function to determine if it's suitable, otherwise undoes the vars and uses bundled.
starseeker (May 23 2020 at 19:52):That's basically what I'm doing - it's easier though to have the FindTCL.cmake report the failure, because all the ThirdParty logic is already set up to do that sort of cleanup
Sean (May 23 2020 at 19:53):I'm suggesting NOT having our own FindTCL ...
Sean (May 23 2020 at 19:54):so our needs are a simple overlay and we aren't tied to whatever snapshot Find script that got used. they can update/change and our function would still do it's job independently. neither should affect the other unless variable names change.
starseeker (May 23 2020 at 19:55):It's a lot harder to do it that way the way the ThirdParty.cmake is currently designed.
Sean (May 23 2020 at 19:56):I don't understand then. Because it's using the system one now, no?
starseeker (May 23 2020 at 19:57):Kinda. It's using a cleaned up version o fthe system version, but our turning on and off of the bundled is keyed on the results of the find_package call.
Sean (May 23 2020 at 19:57):I'm suggesting it keep doing whatever it's doing now, which presumably amounts to setting up a set of variables, maybe some cache values.
Sean (May 23 2020 at 19:58):So it's NOT using the system one.... c'mon clarity and precision here. :)
Sean (May 23 2020 at 19:59):It's using a copy of the system one.
Sean (May 23 2020 at 20:00):okay, I misunderstood you saying that it was switched back to using the standard system cmake find script. it switched to a "less modified" version if I'm understanding correctly now.
starseeker (May 23 2020 at 20:01):OK - in detail:
a) it is using a FindTCL.cmake file in misc/CMake
b) that FindTCL.cmake file is a concatenation and simplification of FindTCL.cmake , FindTclsh.cmake, FindTlStub.cmake, and FindWish,cmake from upstream CMake.
c) This is an improvement over the old FindBRLCADTCL.cmake, which used to be an essentially complete (and complicated) rewrite.
starseeker (May 23 2020 at 20:01):However, because the contents are mostly those upstream files, the original graphics system check that we had in FindBRLCADTCL.cmake was removed.
Sean (May 23 2020 at 20:02):So then we presumably do this for every single Find dependency we rely on?
Sean (May 23 2020 at 20:02):i.e., have modified copies of all the cmake Find scripts, none using the default?
starseeker (May 23 2020 at 20:02):Only if we have to. FindX11.cmake is customized, and FindGL.cmake may still be
starseeker (May 23 2020 at 20:03):I need to audit our current copies against the minimum required CMake copies and see if any can be removed - I haven't checked in quite a while
Sean (May 23 2020 at 20:04):but from what you described, it sounds like we do for the sake of ThirdParty logic
starseeker (May 23 2020 at 20:05):Only if the "default" CMake module behavior is insufficient for what we need.
Sean (May 23 2020 at 20:05):if we can for some, then what's the distinction with Tcl being a lot harder?
Sean (May 23 2020 at 20:05):Is there any example where the default cmake module behavior is sufficient?
starseeker (May 23 2020 at 20:06):ZLIB and PNG are both default now, IIRC.
starseeker (May 23 2020 at 20:06):Hang on, let me check...
Sean (May 23 2020 at 20:07):Sorry, don't mean to be a pain, I'm honestly trying to understand because at heart my understanding is that nearly all Find scripts simply set the location of the lib, headers, maybe a root dir, maybe a couple other vars, but essentially what amounts to the lib and headers are what we need...
Sean (May 23 2020 at 20:07):and this "setting" is really just a set of naming convention variables
starseeker (May 23 2020 at 20:09):The Find modules report out paths and headers, but the more complex ones (Qt I think is an example) also allow callers to specify criteria by which they can accept or reject particular versions or features as inadequate to the build's needs.
The Find module defines required variables, which are set according to tests. Those tests can be simple find_path and find_library looksups, or they can be more complex
starseeker (May 23 2020 at 20:10):Actually, Qt is another case where we don't bundle the Find module
starseeker (May 23 2020 at 20:11):Most of the find modules we have in misc/CMake are actually for things that are too obscure to be bundled with standard CMake (openNURBS, SCL, UTAHRLE, etc.)
Sean (May 23 2020 at 20:12):starseeker said:
The Find modules report out paths and headers, but the more complex ones (Qt I think is an example) also allow callers to specify criteria by which they can accept or reject particular versions or features as inadequate to the build's needs.
This sounds like it'd make it a lot easier, not harder. So.. not a problem.
The Find module defines required variables, which are set according to tests. Those tests can be simple find_path and find_library looksups, or they can be more complex
Sure.. and they are whatever they are.
starseeker (May 23 2020 at 20:15):Right. So if the system tests give us what we need, we can just use the bundled version and no problem. Unfortunately, they're not always adequate - and it used to be that Tcl was one of the most inadequate. The newer logic is much improved from what I remember, but most "proper" Tcl/Tk packages aren't supposed to care what graphics system is in use - so the vanilla system tests for Tcl don't check. (Most everything in the Tcl world that does care uses the TEA build setup, so we're pretty much on our own.)
Sean (May 23 2020 at 20:17):maybe need to back up, because this whole conversation started with a suggestion of using an uncopied unmodified cmake-provided FindTcl ... and then running a function from our misc/CMake to do whatever additional acceptance/rejection criteria we have. You responded that would make something a lot harder. Can you elaborate on that? that's the only point in question that I don't understand because to me it should just be us unsetting vars (rejecting something system cmake thought we could use).
Now if it fails to detect at all and there's nothing to reject, I might see a benefit, but that didn't seem to be what we were talking about.
Sean (May 23 2020 at 20:21):To that point, it looks like in r75899 that you're even doing exactly what I suggested, wiping out all the vars. You're just doing it in the copy instead of doing it in another file that's run after a system Find.
starseeker (May 23 2020 at 20:21):OK. So the guts of the third party management logic are in misc/CMake/ThirdParty.cmake, around line 200. That's when we do the find_package call, using either our local misc/CMake copy if its present or the CMake defined version if we don't give it one.
Once that find_package call is complete, we have a yes/no decision for whether the system version is usable. From that point on, the work beings to set all the necessary cache variables and other fun that go into making the 3rd party build system work the way it does. All of that logic assumes that after the find_package call, the decision is made one way or the other on whether we're going to build 3rd party.
If we want to subsequently change our mind once we're out of that function, all the bookkeeping and state management after the find_package call in that function has to be done again.
Sean (May 23 2020 at 20:23):why wouldn't we just do the validation check in ThirdParty right after find_package?
starseeker (May 23 2020 at 20:24):Note also that the FindTCL.cmake file itself, at the end, has calls to standard CMake functionality FIND_PACKAGE_HANDLE_STANDARD_ARGS that find_package is expected to execute. If we wanted to change our decision post find_package, we'd also have to make sure we weren't messing up anything that would be subsequently relied upon by CMake's state management.
starseeker (May 23 2020 at 20:26):That's why the graphics check is located where it is in that file - so we can make the decision before those finalization routines are called.
It's undoubtedly possible to handle it anyway - a reconfigure with changed build settings actually does do that - but it's a hassle to get right and I've broken things hundreds of times over the years when I've had to muck with that aspect of things.
starseeker (May 23 2020 at 20:27):It's also a common source of the "weird intermediate state" build errors we get when someone does a lot of reconfiguring with different settings without clearing the cache or build directory - if I drop a stitch on the cleanup, weird stale state can start having unpredictable consequences.
Sean (May 23 2020 at 20:31):starseeker said:
That's why the graphics check is located where it is in that file - so we can make the decision before those finalization routines are called.
But then this could still cause the unintended consequences you're trying to avoid I would think. Are you saying there's state somewhere involved beyond the variables and the cache?
It's undoubtedly possible to handle it anyway - a reconfigure with changed build settings actually does do that - but it's a hassle to get right and I've broken things hundreds of times over the years when I've had to muck with that aspect of things.
I don't want to rest on a pile of sand. If it's brittle, that's all the MORE reason it needs to be modularized overlays with gated changes.
Sean (May 23 2020 at 20:36):maybe I should play with some of these ideas, I must not be communicating clearly or I'm just totally not getting it. probably both. To me, this seems like a modularity 101 -- to move towards not having copies and not modifying if we can overlay. The only issue I could see would be a script that is just fundamentally broken or non-existent, so we provide one.
starseeker (May 23 2020 at 20:36):There are internal CMake variables beyond those we normally manipulate - if you search for :INTERNAL in the cache you'll see them. I can't say what they all do - I try not to have to manipulate things at that level - but it's clearly state being tracked by CMake.
The problem is "resetting" a find package result is just going to be brittle no matter what. It's not something that the system is really designed for - the expectation pretty much seems to be that if you want to change things you clear the cache and re-run. We do better than that - if we flip between SYSTEM and BUNDLED the right thing should happen - but it's pretty unnatural from CMake's perspective that we do.
Sean (May 23 2020 at 20:38):at the end of the day, though, it's still just a bunch of variables and if push came to shove, I could write a list before the Find and a list after the Find, and reset... at least I'd hope that's possible.
starseeker (May 23 2020 at 20:40):Sure - the question is how invasive you want to get and at what levels. ResetCache.cmake does some pretty gnarly manipulations, for example
starseeker (May 23 2020 at 20:41):That was an attempt to reset library searching when switching between 32 and 64 bit builds. Don't know if it still works - haven't tried to do that in a long time - but that was one instance of a mean low-level state manipulation attempt.
starseeker (May 23 2020 at 20:42):I've always tried to let CMake do it's own thing as much as possible and roll my own logic on top of it only when necessary (I know it probably doesn't look that way)
Sean (May 23 2020 at 20:43):It's not about being invasive. If anything it's "less invasive" on code. We're cleaning up after the fact to whatever extend necessary to undo a system Find result deemed unacceptable. "you do you, and we'll sort it out after"
Sean (May 23 2020 at 20:44):I wouldn't do something more aggressive than demonstrably required.
Sean (May 23 2020 at 20:44):I have trouble believing that we can't just unset the public vars we end up using.
starseeker (May 23 2020 at 20:46):Well, in that most recent commit I initially tried unset() on the vars - it didn't work. I had to override the CACHE before I got the desired behavior.
Now, could we do that after the find_package call but before the rest of the ThirdParty logic executes? Probably - but I'm not at all sure we wouldn't be just trading one form of complexity for another
starseeker (May 23 2020 at 20:49):If a new version of CMake has FIND_PACKAGE_HANDLE_STANDARD_ARGS start doing some new behind the scenes magic as a consequence of the find decision, we'd have to catch that and mod our after-the-fact cleanup code to also unravel the new behavior. That may not be a huge risk, but I'm not sure how we'd catch it...
starseeker (May 23 2020 at 21:13):At a glance, I think our FindGL.cmake was originally modded mainly to allow us to find X11 OpenGL on the Mac. Not completely sure what upstream does with that case, but looking at the file I think they default to the native OpenGL if(APPLE)
starseeker (May 23 2020 at 21:17):FindX11.cmake I think needed Xinput, plus had a habit of mixing include headers from two different X11 installations on OSX.
starseeker (May 23 2020 at 21:18):Our FindOpenCL.cmake is quite different from the upstream - not sure what the relative merits are.
starseeker (May 23 2020 at 21:51):Ah - unset(<var> CACHE) has the same result, looks like
Last updated: Oct 09 2024 at 00:44 UTC