search

TriBITSPub / TriBITS

TriBITS: Tribal Build, Integrate, and Test System,
http://tribits.org
Other
35 stars 47 forks source link

Move to modern CMake target-based propagation of build information #299

Closed bartlettroscoe closed 1 year ago

bartlettroscoe commented 4 years ago

Parent Issue:

Child Issues:

Blocked By: trilinos/Trilinos#8498

Description

One of the things holding back TriBITS and projects using TriBITS in accessing new features of CMake for which TriBITS still using variables to manage compiler options and include directories. This issue is to move to using:

and to move completely to targets instead of variables for each TriBITS (internal and external) package.

This needs to be done as part of a comprehensive refactoring as part of #63.

This refactoring should implement and respect this proposed standard for <Package>Config.cmake files.

Related to:

Tasks

bartlettroscoe commented 4 years ago

This refactoring will make it easier to integrate Kokkos and KokkosKernels into Trilinos as described in the below email from @jjwilke.

From: Wilke, Jeremiah J jjwilke@sandia.gov Sent: Tuesday, February 4, 2020 4:18 PM To: Wolf, Michael M mmwolf@sandia.gov; Rajamanickam, Sivasankaran srajama@sandia.gov; Bartlett, Roscoe A rabartl@sandia.gov; Willenbring, James M jmwille@sandia.gov; Trott, Christian Robert crtrott@sandia.gov; Teranishi, Keita knteran@sandia.gov Subject: To-do list for Trilinos

Below would be my (very specific) action items to get where we need to be.

• Move to CMake 3.13 (allows link_options and better with flag deduplication) • Change variables to lists instead of strings (where appropriate) • Change include_directories/compile_options to target_include_directories(….) • Remove all dependencies between packages that rely on variables. Only use target_link_libraries(…) to propagate flags

I would guess this could be done in a month? Once this is done, it should be straightforward to design a system where Kokkos could be a TPL.

-Jeremy

bartlettroscoe commented 4 years ago

The Trilinos issue to allow the upgrade of the minimum version of CMake from 3.10 to 3.13 for TriBITS as well is trilinos/Trilinos#6752.

bartlettroscoe commented 4 years ago

@jjwilke, @sethrj,

This brings up another issue. These refactorings of TriBITS to use target-centric features in CMake may make it very difficult to maintain backward comparability with-respect to the build system. For example, it may not be possible keep supporting the generation of Makefile.export.<Package> files for customers like INL MOOSE. Trilinos backward-comparability requirements might make that difficult to allow.

jjwilke commented 4 years ago

I think we can probably make this work? Everything that needs to be in the Makefile will end up in INTERFACE_COMPILE_OPTIONS, INTERFACE_LINK_LIBRARIES, etc. We could probably make a function 

FUNCTION(EXPORT_TARGET target)
...

That doesn't rely on variables like Kokkos_CXX_FLAGS and instead calls get_property to figure everything out.

bradking commented 4 years ago

This will also be relevant to https://github.com/TriBITSPub/TriBITS/issues/127#issuecomment-549912354.

sethrj commented 4 years ago

FWIW, SCALE/Exnihilo is only using the exported tribits config.cmake files for the downstream enrico project, not any makefile.export.

I'm definitely OK with a transition plan for removing backward compatibility gradually 😬

jjwilke commented 4 years ago

I will say (and maybe @bradking already knows how to do this) - the reason I stopped short of exporting a Makefile.kokkos was generator expressions in the property lists.

bradking commented 4 years ago

stopped short of exporting a Makefile.kokkos was generator expressions in the property lists.

Modern CMake target-based usage requirements mean that much of the logic to convert project CMake code into the final build system command-lines is handled by CMake's generators. The input to that process is not exportable to Makefile.*, and the output is only available in generated build systems inside CMake-generated build trees. Easy export to Makefile snippets was not a design goal.

bartlettroscoe commented 4 years ago

Easy export to Makefile snippets was not a design goal.

It occurred to me that it still might be possible to produce Makefile.* files. What you do is you create a dummy CMake project that pulls in TrilinosConfig.cmake or <Package>Config.cmake and then you use your own compiler and linker wrappers to capture the compiler and linker arguments and use that to generate the Makefile.export.* files. That is almost exactly what the ASC Sierra project does with their "bake" tool to generate Makefiles and build.ninja files given output from a Bjam build system. Crude but that would work.

bartlettroscoe commented 4 years ago

FYI: I just got feedback from lead INL MOOSE framework lead that they dropped testing against Trilinos (because CASL has ended) and don't plan on supporting Trilinos going forward (unless something changes). Therefore, because I think we could add support for Makefile.export.* files like a described above, and it is not needed now, I will remove testing and support for this in TriBITS when I do this refactoring. Yea!

bartlettroscoe commented 4 years ago

FYI: It just occurred to me that the SNL Sierra project might be using Makefile.export.Trilinos for its custom bJam/bake build system that uses the native Trilinos TriBITS/CMake build system. I have reached out to them to see if that is the case.

bartlettroscoe commented 4 years ago

FYI: Got back a response from the SNL Sierra DevOps lead that Sierra actually does not use the Makefile.export.Trilinos file. They actually manually maintain a set of BJam files for building Trilinos and they get the info they need from that (even when building Trilinos using the native TriBITS CMake build system).

Therefore, it looks like we don't need to keep generating Makefile.export.Trilinos files based on SNL Sierra needs. That is good news :-)

bartlettroscoe commented 4 years ago

Part of this refactoring is in the PR #300 from @bradking. It removes explicit handing of -std compiler options. It sets the global var CMAKE_CXX_STANDARD and it sets:

TARGET_COMPILE_FEATURES(<libtarget> PUBLIC <cxx_std>)

to that matching standard.

bartlettroscoe commented 4 years ago

Someone just showed me the option CMAKE_EXPORT_COMPILE_COMMANDS which writes a comple_commands.json file after the generate phase is complete. From that you can easily extract the information needed for Makefile.export.* files (but that would have to be created by a build target, after configure and generate were complete). Now to get other things out of the way so I can get into this.

bartlettroscoe commented 4 years ago

FYI: I just realized that PETSc is still using the Makefile.export.Trilinos file to get the list and order of Trilinos libraries. See:

So I think unless PETSc switches over to CMake, we are going to need to re-implement support for at least the Makefile.export.<Project> file as part of this refactoring :-(

sethrj commented 4 years ago

Or just require PETSc to use older trilinos/tribits version until they switch?

bartlettroscoe commented 4 years ago

Or just require PETSc to use older trilinos/tribits version until they switch?

Not sure that is a viable option because of the ECP xSDK and E4S that includes PETSc and Trilinos. But to get by, someone can just manually generate a Makefile.export.Trilinos file in the Spack Trilinos package.py file and that would satisfy PETSc for now for Spack and ECP.

And I have no idea what the timetable would be for PETSc to switch to CMake (or even if that is a firm plan).

CC: @keitat

bartlettroscoe commented 4 years ago

FYI: Looks like the refactored TriBITS may need to need to keep defining the vars TPL_<TPLName>_INCLUDE_DIRS due to code like:

I wonder how much code like this exists in Trilinos and other projects using Trilinos?

rppawlo commented 4 years ago

This issue came up at SART meeting today. At least Xyce is still using export makefiles. They are in the process of transitioning to cmake, but it may be a few months. Will need to coordinate with them. Have sent inquiries to other code teams to see how wide spread the export makefile use is. Will add more info soon.

mperego commented 4 years ago

@bartgol This would affect Albany and the export of the libraries for MPAS, right?

bartlettroscoe commented 3 years ago

FYI: Xyce should be getting some help from Kitware to transition fully to CMake so hopefully the export Makefile requirement for TriBITS and Trilinos can go away completely.

bartlettroscoe commented 3 years ago

FYI: trilinos/Trilinos#8498

bartlettroscoe commented 3 years ago

I guess I am technically starting on this Issue today. I am upgrading TriBITS to require CMake 3.17+ given that Trilinos has already upgraded is min version to 3.17 (see trilinos/Trilinos#8660). This will involve cleaning up a good bit of code and getting rid of various workarounds and hacks for older versions of CMake.

Related to my epic SEPW-214

bartlettroscoe commented 3 years ago

@keitat, do you think that you can have some of the subcontractor staff looking into Trilinos xSDK issues help review PRs related to this and TriBITS changes in general? I desperately need someone to help review TriBIITS changes that really knows CMake well.

keitat commented 3 years ago

Stay tuned. I will let you know the github username of the people from NGA.

bartlettroscoe commented 3 years ago

FYI: It looks like there is an important internal customer that would be significantly impacted if Trilinos looses support for Makefile.export.Trilinos in the next few months (or longer). This refactoirng can't wait any longer so if support for Makefile.export.Trilinos has to be maintained, then we might just have to bit the bullet and implement an approach like they did for ParaView/VTK shown in:

which is outlined in https://github.com/trilinos/Trilinos/issues/8498#issuecomment-825349056 which is basically what I suggested above last year. The fact that Kitware has already implemented this approach for a major project gives me some comfort that this may not be as big of a hack as what I was thinking this was.

But note that this file Makefile.export.Trilinos would get generated at build time, not configure time, and use cases like the integration of INL MOOSE into VERA and demonstrated with the WrapExternal package would not longer work (or would have to work very differently with the configure of the wrapped external project done at build time, not configure time of the outer TriBITS CMake project).

Just goes to show that once an important piece of software acquires a feature that is important to some customer, that you can never remove support for that feature :-(

mathstuf commented 3 years ago

The fact that Kitware has already implemented this approach for a major project gives me some comfort that this may not be as big of a hack as what I was thinking this was.

I wouldn't call it a "big hack", but it is a hack in the sense that it doesn't solve the problem. For example, VTK's object factory stuff is absent there, so that is still manual. There are probably other usage requirements that are missing. If a project has Fortran-specific usage requirements, the synthesized project has to use Fortran for those to be exposed.

Basically, the example project needs to encompass the use cases that exist then have ways of teasing out what is wanted. That's where the "hackiness" comes in: you're making tiny examples then trying to extrapolate it for full usage which doesn't always work. I would stick huge disclaimers on this that CMake is the preferred way to use Trilinos and that this exporting mechanism is a best-effort attempt at bridging the gap.

bartlettroscoe commented 3 years ago

FYI: That internal SNL customer still using Makefile.export.* files for a daily integration with Trilinos 'master' is going to try switching that build over to using their new CMake build system for their application code. They will try to get that done in the next few weeks. Then they will plan to officially switch to CMake for all of their builds before their next major upgrade of Trilinos based on the current Trilinos 'master' branch.

Therefore, I need to wait until they give me the green light before I merge a version of TriBITS to Trilinos 'develop' that drops support for Makefile.export.* files.

bartlettroscoe commented 3 years ago

FYI: After talking with that internal customer still using Makefile.export.* we may be able to switch them over to use CMake for their daily integration builds against Trilinos 'master' and avoid needing to support Makefile.export.*, even in the short-term. For details, see https://github.com/trilinos/Trilinos/issues/8498#issuecomment-834681399. Fingers crossed that transition to CMake for the customer will be completed soon so we are not blocked from dumping support for Makefile.export.* and get this TriBITS refactoring to use modern CMake done. But we can't push an updated version of TriBITS that drops support for Makefile.export.* to Trilinos 'mastesr' until that customer's daily APP 'master' + Trilinos 'develop' integration build is switched over to use CMake. I just don't want to be doing this work on another long-lived branch of TriBITS like I did with the branch 63-merge-tpls-packages that still is not merged in (but will be soon since it should not break backward compatibility).

bartlettroscoe commented 3 years ago

Good news. As stated in https://github.com/trilinos/Trilinos/issues/8498#issuecomment-850503201, it looks like we can finally drop support for Makefile.export.* files from Trilinos (and therefore TriBITS)! I promised to keep support in the Trilinos 'develop' through 5/10/2021 but that is just a little under 2 weeks from now.

I realized that I can do a lot of the refactoring to create the new targets supporting modern CMake incrementally and safety without breaking backward compatibility initially to see how this works before actually needing to drop support for Makefile.export.*. For example, I can create targets <PackageName>::all_libs for all of the TPLs and packages and ensure these targets actually work to link executables in a dummy TriBITS-lite example/test project.

So given where I am with the recent refactorings/cleanups in #63 as part of epic #367, I am ready to start extending TriBITS to provide fully modern CMake targets as per this proposed standard next week!

bartlettroscoe commented 2 years ago

@marcinwrobel1986, see commit 3ac791bcf0cec15bf8753287c9ba475262e88fa5. We need to test with Trilinos and a (dummy) customer APP. Perhaps we can meet offline briefly for details if you have time and funding for this?

bartlettroscoe commented 2 years ago

Below I summarize an email chain and then a meeting I had Ben Boeckel of Kitware about how to deal with a difficult issue related to moving to self-contained modern CMake targets which includes for external packages / TPLs where the TPLs may be specified as just a list of link flags and there may be upstream dependencies. There is a workable implementation path that I will summarize below. The motivating email chain is below that.

The problem is that target_link_libraries() allows the user to pass in link options like:

target_link_libraries(<libName> -L<dir> -l<lib> ...)

and TriBITS allowed users to specify TPL libraries in that way as described here which warned not to do:

TPL_SomeTPL_LIBRARIES="-L/some/dir;-llib1;-llib2;..."

But a lot of people's Trilinos configure scripts did just this.

But to create self-contained library targets for these external packages/TPLs that know their upstream dependencies as well, we need to create <tplName>Config.cmake files that contain imported targets. The way to do this is to take advantage of the target property IMPORTED_LIBNAME, which when combined with INTERFACE_LINK_OPTIONS, allows you to take a list of link arguments like:

   -L<dir> -l<lib>

and define and IMPORTED INTERFACE library that allows passing these options to the link line with -l<lib> in the right location.

I experimented with this in the PR #412 commit https://github.com/TriBITSPub/TriBITS/pull/412/commits/e11ac0e17ebd599fd940016aca54f6454157a2e2 and it seemed to work as it should.

By parsing the list of options one should be able to separate out the -L<dir> and -l<lib> parts in a set of options like:

  -L<dir1> -l<lib1> -L<dir2> -l<lib2> …

and create a set of IMPORTED INTERFACE libraries, one target <tplName>::<libi> library for each -l<libi> option, and then create an INTERFACE library target <tplName>::all_libs that links against these and sets the -L<diri> arguments with target_link_options() to build the full link line. You can see where I successfully experimented with that in the PR #412 commit https://github.com/TriBITSPub/TriBITS/pull/412/commits/c5962b87e9acbc752576e5c53399094362547b83.

However, this is not a 100% bulletproof solution because it is possible that other link options can exist like -Wl,-Bstatic or -Wl,Bdymanic that would not be able to be handled with IMPORTED_LIBNAME but I think these are pretty rare and we could warn or error out if we encounter link options other than start with anything other than -L or -l (or are a full lib path).

Looking at the ATDM Trilinos configuration files under:

   Trilinos/cmake/std/atdm/

using:

   $ cd Trilinos/cmake/std/adm/
   $ find . -type f -exec grep -nH "[ ;\"][-]l" {} \; | grep -v "~:" | fold -w 180 | less

I can’t find a single example where a TPL is specified with anything other than -L<dir>, -l<lib> or a complete library path.

Therefore, with this, I believe we have a solution that can handle 99% or more of the configuration scripts out there for Trilinos (and 100% of the ATDM Trilinos configurations) and allow the creation of fairly straightforward <tplName>Config.cmake files for TriBITS TPLs with IMPORTED and INTERFACE targets for external packages/TPLs where there are dependencies on each other and there are some TPLs that are specified as a set of -L<dir> and -l<lib> options.

Below, is the email chain in this conversation.

From: Boeckel, Ben Sent: Thursday, September 9, 2021 9:17 AM
> configures just fine with -l and -L. It is only at build time that it > fails when the build rule can’t find the “file” with “`-l` > `-L`”. (But it builds just fine if you make a few small > adjustments to the generated files to remove those dependence rules > and remove quotes from the link line.) Oh, the location is assumed to be a single argument and is passed along that way (the path may have spaces in it, so the "obvious" solution isn't going to work here). > * This makes sense and already exists: `INTERFACE_LINK_OPTIONS`. > > Nope, I already tried an experiment with that. See: > > > https://github.com/TriBITSPub/TriBITS/pull/412/commits/ca360fe52794187 > 53ae3dd2df5d2fc33a51ecb9b > > The problem is that CMake puts INTERFACE_LINK_OPTIONS before all of > the object files and libs on the link line. (I can’t seem to find any > documentation in CMake that defines the ordering of these things on > the link line. You only seem to be able to determine this by trial > and error with CMake. Does does CMake document the ordering of the > exact compile and links lines for all of this stuff?) Oh, so this needs more control over the link flags. However, I think for `-l` flags, making imported targets would let CMake have a better idea of what you're doing.
From: Bartlett, Roscoe A Sent: Thursday, September 9, 2021 9:00 AM
Hello Ben, Just a few small responses … > Icky, no :) . This is very non-relocatable. For the scenarios where we need to get this to work, we don’t care about relocatable anything. It just needs to support that one machine and one non-relocatable install on those machines. (The machines where we have to use link options -l -L are nasty HPC machines.) > Eh, I think `IMPORTED_LOCATION` gets `if (EXISTS)` applied to it at some level. I don't know where though, so this may be "dangerous". No, it configures just fine. There appears to be no configure-time checks that those files actually exist. For example, the experiment in the commit: * https://github.com/TriBITSPub/TriBITS/pull/412/commits/9147e8e58c15e910b37090a88dfbdc038acaefef configures just fine with -l and -L. It is only at build time that it fails when the build rule can’t find the “file” with `“-l -L”`. (But it builds just fine if you make a few small adjustments to the generated files to remove those dependence rules and remove quotes from the link line.) > This makes sense and already exists: `INTERFACE_LINK_OPTIONS`. Nope, I already tried an experiment with that. See: * https://github.com/TriBITSPub/TriBITS/pull/412/commits/ca360fe5279418753ae3dd2df5d2fc33a51ecb9b The problem is that CMake puts INTERFACE_LINK_OPTIONS before all of the object files and libs on the link line. (I can’t seem to find any documentation in CMake that defines the ordering of these things on the link line. You only seem to be able to determine this by trial and error with CMake. Does does CMake document the ordering of the exact compile and links lines for all of this stuff?) Let’s talk over this in person (over MS Teams). I think if CMake could support something like IMPORTED_LIBRARY_LINK_OPTIONS (where those options are inserted into the same location on the link like where a library files would be inserted using IMPORTED_LOCATION) then it would make this a piece of cake (or at least go more smoothly). -Ross
From: Boeckel, Ben (External Contacts) Sent: Wednesday, September 8, 2021 10:00 PM
On Tue, Sep 07, 2021 at 14:55:25 +0000, Bartlett, Roscoe A wrote: > First, do you mind if I post this past email chain and your future > responses to the public GitHub issue > https://github.com/TriBITSPub/TriBITS/issues/299? Sure, I don't think there's anything secret here. > The problem I am having is that I need to be able to take the existing > TriBITS TPL > specification https://tribits.org/doc/TribitsBuildReference.html#enabl > ing-support-for-an-optional-third-party-library-tpl > which should be a list of library files but may also be nothing more > than a set of `-l`, `-L` and some other link options and be > able to create CMake targets and an aggregate target > `::all_libs` that other downstream internal targets can link > against (and to define link dependencies between TPLs to define link > ordering). And then when I install the `Config.cmake` files > for internally-build TriBITS CMake packages, the IMPORTED targets > those files need to find those `::all_libs` targets somehow. I > was hoping that I could do this with `add_library( … INTERFACE … )` > targets but I don’t seem to be able to tell CMake about the proper > ordering between TPLs in this way where there are intermediate CMake > targets. I seem to only be able to define the ordering dependencies > correctly if they are imported libraries created with `add_library( … > IMPORTED GLOBAL …)` and set the IMPORTED_LOCATION property (which you > must to give it a full library file path, not a set of link options). Generally, you would want each library to be its own imported library rather than trying to bundle these things together. You can then bundle with an `IMPORTED INTERFACE` `all_libs` target however. For another discussion I had similar to this, see this Discourse post (and its thread): https://discourse.cmake.org/t/config-modules-and-recursive-import-target-dependencies/3735/4 > I need this to figure out how to make existing TriBITS TPLs define > standard targets `::all_libs` and define dependencies between > these TPL targets so that they can be used in downstream native CMake > targets just like other targets. This is needed to achieve a system > where packages can be built and pre-installed up-front, or TPLs can be > pulled in and built as native CMake TriBITS packages, or other > scenarios. This is needed to create a system where internal packages > (i.e. TriBITS packages) and external packages (i.e. TriBITS TPLs) can > be interchangeable, even when the external packages were not initially > specified using a `Config.cmake` file with proper > IMPORTED library targets and there are external packages/TPLs are only > specified as a set of include directories and a set of link options > (that must be put in a specific order on the link line). What I would recommend is something like `PkgConfig` since this is actually really close to what you're doing. This API would live in `FindTriBITSPackage.cmake` and can be used by doing `find_package(TriBITSPackage)`. This would provide an API to create imported targets from TriBITS packages as the spec desires: ``` cmake find_package(TriBITSPackage REQUIRED) find_tribits_package(SomeName VERSION 1.2 REQUIRED) # `SomeName::all_libs` is an IMPORTED INTERFACE target which exposes all # libraries from `SomeName` which are available individually as # `SomeName::libA` and `SomeName::libB`. If `SomeName` has dependencies, # these are pulled in via a recursive `find_tribits_package()` call as # needed. Note that `if (NOT TARGET …)` guards will be needed to avoid # double-declaring targets. ``` When exporting this in a CMake package, the process is just repeated with some extra logic to thread around the `FOUND` variable setting: ```cmake find_package(TriBITSPackage) if (TriBITSPackage_FOUND) find_tribits_package(SomeName VERSION 1.2) if (NOT SomeName_FOUND) set(CurrentPackage_FOUND 0) list(APPEND CurrentPackage_NOT_FOUND_MESSAGE "TriBITS package `SomeName` is not available: ${SomeName_NOT_FOUND_MESSAGE}") endif () else () set(CurrentPackage_FOUND 0) list(APPEND CurrentPackage_NOT_FOUND_MESSAGE "TriBITSPackage is required to find TriBITS-built dependencies") endif () ``` > Unless you guys can show me something about CMake that I have not been > able to figure out, I will need to restrict the specification of TPLs > as follows: > > * 1) External packages/TPLs with one or more upstream dependencies > must either: > * A) Define all of their parts as library files and include > directories (so we can create IMPORTED libraries with > proper dependencies), or I think just re-discovering them at `find_package` time is best. It allows the dependencies to have relocated since the build occurred (e.g., Spack, ABI-compatible updates, etc.). > * B) Define the library using link flags but also have to > provide the link flags and libraries for all upstream TPLs > as well (and then we will use a single INTERFACE library > for `::all_libs)` (But I think I could relax this > requirement and have TriBITS do this automatically > internally but it will be a bit complex.) Icky, no :) . This is very non-relocatable. > * 2) External packages/TPLs with zero upstream dependencies are no > problem since: > * A) If all of the libraries are specified as files, then > simple IMPORTED library targets can be created. > * B) Otherwise, if any link options are used, then a single > INTERFACE library for `::all_libs` will be created > (and since there are no upstream CMake targets, there is > no ordering issue). Why not attach the link options to the individual targets and let the usage requirements of `all_libs` forward them through? > The problem is that I know for a fact that the requirement 1.B will > break some existing Trilinos configure scripts that specify some > downstream TPLs using linker flags and don’t list the link flags for > all upstream TPLs in the same TPL var. For example, current TriBITS > will put the link libraries in order for all of the enabled TPLs for a > package and many packages use BLAS and LAPACK and it is often that > BLAS is specified as ‘-lblas’ and LAPACK is specified as ‘-llapack’. > Currently TriBITS knows the ordering of these two TPLs and will sort > them and build the link line correctly so you get: > > ` … -llapack -lblas …` > > If we strip out all of this TriBITS logic for sorting and ordering > packages and TPLs and just expect the modern CMake targets to know > their proper dependencies, then from my experiments with CMake with > IMPORTED libs, we may get: > > ` … -lblas -llapack …` > > which will fail the link if these are static libs. The fix here would be to have the `lapack`-representing target link to the `blas`-representing target. CMake has support for duplicating static libraries on the link line for symbol resolution (if it is known they are STATIC rather than UNKNOWN). To detect shared versus static, see this MR for logic that works on the platforms you probably care about at least: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/5820 > I would really like to get rid of all of the complex TriBITS code that > has to globally sort TPLs according to their global/relative order. I can understand that. I think just using `find_package` and letting `if (NOT TARGET)` handle the duplication should at least let CMake figure out the flag sorting for you. > NOTE: This problem could be addressed if CMake added a new target > property like IMPORTED_LIBRARY_LINK_OPTIONS to replace > IMPORTED_LOCATION in the case where you accept arbitrary link options > instead of a single file. You would use it like this: Exported link options show up *somewhere* right? Looking at VTK (which exports link options), they show up on an `INTERFACE IMPORTED` target under the `INTERFACE_LINK_OPTIONS` property. I think all of the usage requirement things show up as `INTERFACE_` properties and things specific to IMPORTED targets start with `IMPORTED_`. > `add_library( STATIC IMPORTED GLOBAL)` Please don't use `GLOBAL` forcefully here. If it is truly wanted, projects can promote specific targets by setting the `GLOBAL` property to `TRUE` manually. It shouldn't be necessary though. > `set_property(TARGET PROPERTY IMPORTED_LOCATION "-l -L")` The imported location really should be a full path to the library itself. I just had an issue I had to track down recently where the `gfortran` we use on macOS needed to be told `-L/usr/lib` manually to find `libSystem.tbd`. Alas, `libblas.tbd` and `liblapack.tbd` exist there to point to `Accelerate.framework`. If these libraries had been consumed via full paths, there wouldn't have been any confusion as to why these are interposing on the blas/lapack libraries we had built when the build discovered the intended blas/lapack at the beginning of the build. Instead, it used `-L` and `-l`, but at the wrong position compared to `$FFLAGS` so that they didn't actually work as intended. `find_library(NAMES ${lname} HINTS ${Ldir} NO_DEFAULT_PATH)` would allow you to get the library name if you're not sure what the extension/prefix should be. > I basically validated that this would work in my experiment in commit: > > > https://github.com/TriBITSPub/TriBITS/pull/412/commits/9147e8e58c15e91 > 0b37090a88dfbdc038acaefef > > In that experiment, I set IMPORTED_LOCATION to `“-l -L”` and > manually modified the generated build.make and link.txt files to do > what needs to be done in that case and it worked perfectly to create > the correct link line. Eh, I think `IMPORTED_LOCATION` gets `if (EXISTS)` applied to it at some level. I don't know where though, so this may be "dangerous". > CMake would handle IMPORTED_LIBRARY_LINK_OPTIONS identically to > IMPORTED_LOCATION except: > > 1. It would not put in Makefile/Ninja dependencies from > IMPORTED_LIBRARY_LINK_OPTIONS to the targets that depend on the > IMPORTED target (because IMPORTED_LIBRARY_LINK_OPTIONS is not a > file) > 2. It would not quote the entire string > IMPORTED_LIBRARY_LINK_OPTIONS when putting it on the link line > (but might quote each element in the option list perhaps). This makes sense and already exists: `INTERFACE_LINK_OPTIONS`. > I think this it would take just a few lines of C++ code in CMake to > support this (and 100x as much documentation and testing of course). Or zero new lines :) . > Otherwise, I think it will be very difficult to refactor TriBITS such > that: > > 1. TriBITS uses modern CMake targets for all linking and handling > of include dirs, etc. > 2. Have TriBITS handle external packages (TPLs) and internal > packages in a uniform way > 3. Delete a bunch of complex TriBITS sorting code > 4. Maintain backward compatibility for existing Trilinos > configurations of existing Trilinos configuration scripts on all > platforms Yeah, without that, it does sound difficult. However, I think that per-library imported targets is just going to be more flexible here. > But given the tradeoffs, I think I would accept more complexity in the > generation of (hacked up) `Config.cmake` files which would then > allow for correct `::all_libs` targets to allow simple > definition of downstream libs and execs. Then package developers can > switch to use raw CMake to create targets internally if they want to > do so. (Therefore, I think I have a workaround, but it will not be > pretty.) Agreed. If you want or need any inspiration/checking of corner cases, take a look at these functions in this file: https://gitlab.kitware.com/vtk/vtk/blob/master/CMake/vtkModule.cmake - vtk_module_find_package - vtk_module_export_find_packages > Can we set up a meeting to discuss the problem I am trying to solve > here and how CMake might be able to support this? Sure, though my calendar is packed for the rest of this week it seems. Monday is relatively free though. --Ben
From: Bartlett, Roscoe A Sent: Tuesday, September 7, 2021 10:55 AM
Hello Ben, First, do you mind if I post this past email chain and your future responses to the public GitHub issue https://github.com/TriBITSPub/TriBITS/issues/299? The problem I am having is that I need to be able to take the existing TriBITS TPL specification which should be a list of library files but may also be nothing more than a set of `-l`, `-L` and some other link options and be able to create CMake targets and an aggregate target `::all_libs` that other downstream internal targets can link against (and to define link dependencies between TPLs to define link ordering). And then when I install the `Config.cmake` files for internally-build TriBITS CMake packages, the IMPORTED targets those files need to find those `::all_libs` targets somehow. I was hoping that I could do this with add_library( … INTERFACE … ) targets but I don’t seem to be able to tell CMake about the proper ordering between TPLs in this way where there are intermediate CMake targets. I seem to only be able to define the ordering dependencies correctly if they are imported libraries created with add_library( … IMPORTED GLOBAL …) and set the IMPORTED_LOCATION property (which you must to give it a full library file path, not a set of link options). I need this to figure out how to make existing TriBITS TPLs define standard targets `::all_libs` and define dependencies between these TPL targets so that they can be used in downstream native CMake targets just like other targets. This is needed to achieve a system where packages can be built and pre-installed up-front, or TPLs can be pulled in and built as native CMake TriBITS packages, or other scenarios. This is needed to create a system where internal packages (i.e. TriBITS packages) and external packages (i.e. TriBITS TPLs) can be interchangeable, even when the external packages were not initially specified using a <`ExternalPackage>Config.cmake` file with proper IMPORTED library targets and there are external packages/TPLs are only specified as a set of include directories and a set of link options (that must be put in a specific order on the link line). Unless you guys can show me something about CMake that I have not been able to figure out, I will need to restrict the specification of TPLs as follows: • 1) External packages/TPLs with one or more upstream dependencies must either: o A) Define all of their parts as library files and include directories (so we can create IMPORTED libraries with proper dependencies), or o B) Define the library using link flags but also have to provide the link flags and libraries for all upstream TPLs as well (and then we will use a single INTERFACE library for `::all_libs`) (But I think I could relax this requirement and have TriBITS do this automatically internally but it will be a bit complex.) • 2) External packages/TPLs with zero upstream dependencies are no problem since: o A) If all of the libraries are specified as files, then simple IMPORTED library targets can be created. o B) Otherwise, if any link options are used, then a single INTERFACE library for `::all_libs` will be created (and since there are no upstream CMake targets, there is no ordering issue). The problem is that I know for a fact that the requirement 1.B will break some existing Trilinos configure scripts that specify some downstream TPLs using linker flags and don’t list the link flags for all upstream TPLs in the same TPL var. For example, current TriBITS will put the link libraries in order for all of the enabled TPLs for a package and many packages use BLAS and LAPACK and it is often that BLAS is specified as ‘-lblas’ and LAPACK is specified as ‘-llapack’. Currently TriBITS knows the ordering of these two TPLs and will sort them and build the link line correctly so you get: ` … -llapack -lblas …` If we strip out all of this TriBITS logic for sorting and ordering packages and TPLs and just expect the modern CMake targets to know their proper dependencies, then from my experiments with CMake with IMPORTED libs, we may get: ` … -lblas -llapack …` which will fail the link if these are static libs. I would really like to get rid of all of the complex TriBITS code that has to globally sort TPLs according to their global/relative order. NOTE: This problem could be addressed if CMake added a new target property like IMPORTED_LIBRARY_LINK_OPTIONS to replace IMPORTED_LOCATION in the case where you accept arbitrary link options instead of a single file. You would use it like this: ``` add_library( STATIC IMPORTED GLOBAL) set_property(TARGET PROPERTY IMPORTED_LOCATION "-l -L") ``` I basically validated that this would work in my experiment in commit: https://github.com/TriBITSPub/TriBITS/pull/412/commits/9147e8e58c15e910b37090a88dfbdc038acaefef In that experiment, I set IMPORTED_LOCATION to `“-l -L”` and manually modified the generated build.make and link.txt files to do what needs to be done in that case and it worked perfectly to create the correct link line. CMake would handle IMPORTED_LIBRARY_LINK_OPTIONS identically to IMPORTED_LOCATION except: 1) It would not put in Makefile/Ninja dependencies from IMPORTED_LIBRARY_LINK_OPTIONS to the targets that depend on the IMPORTED target (because IMPORTED_LIBRARY_LINK_OPTIONS is not a file) 2) It would not quote the entire string IMPORTED_LIBRARY_LINK_OPTIONS when putting it on the link line (but might quote each element in the option list perhaps). I think this it would take just a few lines of C++ code in CMake to support this (and 100x as much documentation and testing of course). Otherwise, I think it will be very difficult to refactor TriBITS such that: a. TriBITS uses modern CMake targets for all linking and handling of include dirs, etc. b. Have TriBITS handle external packages (TPLs) and internal packages in a uniform way c. Delete a bunch of complex TriBITS sorting code d. Maintain backward compatibility for existing Trilinos configurations of existing Trilinos configuration scripts on all platforms But given the tradeoffs, I think I would accept more complexity in the generation of (hacked up) `Config.cmake` files which would then allow for correct `::all_libs` targets to allow simple definition of downstream libs and execs. Then package developers can switch to use raw CMake to create targets internally if they want to do so. (Therefore, I think I have a workaround, but it will not be pretty.) Can we set up a meeting to discuss the problem I am trying to solve here and how CMake might be able to support this? Cheers, -Ross
From: Boeckel, Ben Sent: Thursday, August 26, 2021 1:55 PM
On Thu, Aug 26, 2021 at 17:33:14 +0000, Bartlett, Roscoe A wrote: > Thanks for your quick reply. I have a few follow-up questions. > > * Another difference is that IMPORTED libraries cannot be > `install(EXPORT)`'d while INTERFACE libraries used and exposed in > target interfaces must be installed manually. > > What do you mean by this? Can you give a simple example so I can > understand the issues here? When installing your project, you need to make all of the targets available when another project finds yours. You `install(TARGETS)` all of your own targets and call `find_package` again to get your external targets satisfied (they may have moved/changed since the build, so you can't just bake in build-time paths). > * if your target is made via `find_package` -> always `IMPORTED` > * if your target is not under `find_package` -> never `IMPORTED` > > What do you mean by “under ‘find_package’”? Can you give a simple example? If you're in `Find….cmake` or under a `…-config.cmake` (or `…Config.cmake` if that's your naming preference), IMPORTED targets should be all that are made (APIs you define can make regular targets, but they should be called from client code). > * You can relax the second rule if you don't install any SDK and > instead have a "product". > > What is your definition of an “SDK” vs. a “Product” here? If you install headers, libraries, etc. for use in other code (CUDA), that's an SDK. A "product" stands alone (e.g., MS Office). --Ben
From: Bartlett, Roscoe A Sent: Thursday, August 26, 2021 1:33 PM
Hello Ben, Thanks for your quick reply. I have a few follow-up questions. > Another difference is that IMPORTED libraries cannot be `install(EXPORT)`'d while INTERFACE libraries used and exposed in target interfaces must be installed manually. What do you mean by this? Can you give a simple example so I can understand the issues here? > if your target is made via `find_package` -> always `IMPORTED` > if your target is not under `find_package` -> never `IMPORTED` What do you mean by “under ‘find_package’”? Can you give a simple example? > You can relax the second rule if you don't install any SDK and instead have a "product". What is your definition of an “SDK” vs. a “Product” here? Thanks, -Ross
From: Boeckel, Ben Sent: Thursday, August 26, 2021 12:03 PM
On Thu, Aug 26, 2021 at 15:49:36 +0000, Bartlett, Roscoe A wrote: > What is the practical difference between a library target for an > external library being created with `add_library( ... INTERFACE ...)` vs > `add_library( ... IMPORTED ...)`? It seems you can define a library > target for one or more libraries on the system using: > > `add_library( INTERFACE GLOBAL)` > `target_link_libraries( INTERFACE -l -L > What is the downside of that vs using an standard imported library > target that you have to set the specific path to? Non-INTERFACE IMPORTED libraries have a LOCATION (and an IMPLIB on such platforms). CMake errors at configure time if these don't actually exist. An INTERFACE library won't error until later (either link time or compile time due to missing includes). Additionally, IMPORTED libraries can be SHARED or STATIC. Another difference is that IMPORTED libraries cannot be `install(EXPORT)`'d while INTERFACE libraries used and exposed in target interfaces must be installed manually. Rule of thumb: - if your target is made via `find_package` -> always `IMPORTED` - if your target is not under `find_package` -> never `IMPORTED` You can relax the second rule if you don't install any SDK and instead have a "product". --Ben
From: Bartlett, Roscoe A, Sent: Thursday, August 26, 2021 11:50 AM, To: Brad King; Boeckel, Ben Zack Galbreath, Subject: What is the practical difference between an imported library and an interface library?
Hello Brad, Ben, and Zack, What is the practical difference between a library target for an external library being created with `add_library( … INTERFACE …)` vs `add_library( … IMPORTED …)`? It seems you can define a library target for one or more libraries on the system using: ``` add_library( INTERFACE GLOBAL) target_link_libraries( INTERFACE -l -L
bartlettroscoe commented 2 years ago

FYI: TriBITS PR #424 converts internal TriBITS targets to use modern CMake and the updated internal targets and generated <Package>Config.cmake files mostly satisfies the proposed convention in #342. As noted in PR #424, I still need to address the handling of external packages/TPLs before I can rip out all of the old complex TriBITS logic and complete this issue #299 but I have a pretty good plan for that and my recent experiences gives me hope that will go okay.

Now if I could just find someone to review the documentation and examples in PR #424 and get some early feedback.

bartlettroscoe commented 2 years ago

FYI: Related to #340, as described in #427, there are issues with TriBITS-generated <tplName>Config.cmake files when FindTPL<tplName>Config.cmake modules call find_package(<tplName>). One issue raised in #427 is that find_package() will look under CMAKE_INSTALL_PREFIX. That is a tricky issue that I will need to address somehow.

bartlettroscoe commented 2 years ago

CC: @mperrinel, @marcinwrobel1986, @keitat

While debugging the errors with the updated TriBITS with Trilinos with Albany I came across a bit of an issue. The problem is that it seems that application codes like Albany have not been always following the suggested usage of pulling in libraries using the variables <PackageName>_LIBRARIES but instead have been, in some cases, directly linking to the non-namespaced IMPORTED targets for libraries that are defined by the found <Package>Config.cmake files (included by the TrilinosConfig.cmake file through find_package(Trilinos)). For example, Albany/src/CMakeLists.txts had code like:

https://github.com/sandialabs/Albany/blob/4e50f9bb54c88d1f589be232f363599a8dfd578c/src/CMakeLists.txt#L196-L203

showing:

add_executable(xml2yaml utility/xml2yaml.cpp)
...
target_link_libraries(xml2yaml teuchosparameterlist)
...
target_include_directories(xml2yaml SYSTEM PUBLIC
                          "${Trilinos_INCLUDE_DIRS};${Trilinos_TPL_INCLUDE_DIRS}")

When the old TriBITS implementation, teuchosparameterlist is an actual IMPORTED target and CMake uses this to create a successful link line for the xml2yaml executable in the Albany build system.

But with the new TriBITS, there is no non-namespaced IMPORTED target called teuchosparameterlist (it is the namespaced target TeuchosParameterList::teuchosparameterlist). Therefore, when CMake processes the line

target_link_libraries(xml2yaml teuchosparameterlist)

with the updated <Package>Config.cmake files for the updated TriBITS, there is no existing target of that name. And in this case, CMake assumes that teuchosparameterlist is just some dumb library out on the system and it puts a -lteuchosparameterlist on the link line as demonstrated in the detailed debugging in Detailed debugging notes in:

This is a very "helpful" feature of CMake's target_link_libraries() command as documented under the section A plain library name at:

The fix for this is simple. Applications should just have used the proper TriBITG-defined variables ${<Package>_LIBRARIES} and linked against that instead as shown in the PR:

in lines:

https://github.com/sandialabs/Albany/blob/a4ddf552088a876f908b0721010d38feca0d6a4a/src/CMakeLists.txt#L200-L207

showing:

add_executable(xml2yaml utility/xml2yaml.cpp)
...
target_link_libraries(xml2yaml PUBLIC ${TeuchosParameterList_LIBRARIES})
...
target_include_directories(yaml2xml SYSTEM PUBLIC
  "${TeuchosParameterList_INCLUDE_DIRS};${TeuchosParameterList_TPL_INCLUDE_DIRS}")

And that works with both the old and the new TriBITS.

The variables:

are the TriBITS interface that I have been targeting maintaining (and the PR https://github.com/sandialabs/Albany/pull/778 and the testing associated with that demonstrates that still works).

I think the problem here is that the APP teams did not know how to find Trilinos using find_package(Trilinos) and then just link against a subset of the packages that are found. Therefore, they just guessed, and in the case of Albany, grabbed the raw targets instead of linking against the proper varaibles.

The issue is that when Trilinos support for TrilinosConfig.cmake and the <Package>Config.cmake files was first created (by developers other than me), on one documented how they were supposed to be used or how customer APPs/packages were supposed to link against just a subset of package libraries. I did not get involved in writing this documentation until just last year and by that point the ship had sailed for all of these APPs.

So that this point, we have one of two options to move forward from here:

If we go with option-1, then that should provide better (hopefully about perfect) backward compatibility. But then how do we later remove these non-namespaced targets and help the application codes to move to the new proper namespaced targets?

If we go with option-2, then the APPs will experience nasty build errors like was seen for Albany as demonstrated in:

and it is not clear what is happening.

Note that once the updated TriBITS is merged to Trilinos, then APPs can just link against <PackageName>::all_libs and can stop using the include dirs altogether. That would make this Albany/src/CMakeLists.txt code just be:

add_executable(xml2yaml utility/xml2yaml.cpp)
...
target_link_libraries(xml2yaml PUBLIC TeuchosParameterList::all_libs)

To decide between option-1 and option-2, I need to get advice from some experienced CMake people to get their opinions on how to proceed. I will ask the NextGen people and my Kitware contractors as well.

In the meantime, I will see if this issue might be impacting Nalu as well (which is another APP with a public Git repo).

bartlettroscoe commented 2 years ago

Okay, I just found that there is DEPRECATION target property for CMake 3.17+:

With that, we could go with option-1 above and deprecate these old non-namespaced CMake targets so that, at configure time, if someone tried to link against a deprecated IMPORTED target, then it would generate a warning.

Given this discovery, I think that is the way to go with option-1 and set the DEPRECATION target property. These <libname> targets would be ALIAS targets against the proper namespaced targets <Package>::<libname>. The question is, how do we generate those targets in the <Package>Config.cmake files?

mathstuf commented 2 years ago

But then how do we later remove these non-namespaced targets and help the application codes to move to the new proper namespaced targets?

Make it depend on the requested version of Trilinos (or the package in question). If that is at least the version which provides the new targets, do not create the backwards compat bits. If it is explicitly lower, consider dropping the DEPRECATION property (because that target needs to be used to work across the requested version range). After "enough time" (i.e., when "everyone" could have been expected to migrate), this deprecation can become unconditional. If it is left empty, always add the DEPRECATION property.

Given this discovery, I think that is the way to go with option-1 and set the DEPRECATION target property. These targets would be ALIAS targets against the proper namespaced targets ::.

This sounds reasonable.

The question is, how do we generate those targets in the Config.cmake files?

There's the list of libraries that goes into all_libs already? Can't that be reused?

bradking commented 2 years ago

I agree with option-1 for the reasons posted above. However, option-1 and option-2 are not mutually exclusive. Even if Trilinos were not changing to the namespaced targets, the application teams should still replace their direct non-namespaced target usage with ${<Package>_LIBRARIES} to avoid using undocumented names. The variable references will then work with old versions of Trilinos too. A deprecation warning from option-1 will help teams implement option-2 if they don't want to raise their minimum required version of Trilinos.

bartlettroscoe commented 2 years ago

@mathstuf,

There's the list of libraries that goes into all_libs already? Can't that be reused?

Yes, I know how to get that list but I was hopping that I could just get CMake to generate these targets automatically with the export() command. But given how the targets are defined and used currently using the namespacing, I am not sure If CMake will let me do this. I will experiment with this and see what CMake will allow me to do. If nothing else, I think I know how to create these ALIAS DEPRECATION non-namespaced targets directly in the generated <Package>Config.cmake file. (But again, I would rather if CMake would just do this for me with the right arguments to the right commands.)

@bradking,

Even if Trilinos were not changing to the namespaced targets, the application teams should still replace their direct non-namespaced target usage with ${<Package>_LIBRARIES} to avoid using undocumented names. The variable references will then work with old versions of Trilinos too.

Right, but the question is if we should expect non-compliant customers to refactor their CMakeLists.txt files twice. Once to refactor from the current non-namespaced targets:

target_link_libraries(xml2yaml teuchosparameterlist)

to the variables ${<Package>_LIBRARIES} :

target_link_libraries(xml2yaml PUBLIC ${TeuchosParameterList_LIBRARIES})

and then a second time to remove usage of variables and go with modern namespaced CMake targets <Package>::all_libs by changing to:

target_link_libraries(xml2yaml PUBLIC TeuchosParameterList::all_libs)

(or Trilinos::TeuchosParamterList_libs as per DRAFT: Proposed Standard for PackageConfig.cmake files for inter-package Interoperability).

bartlettroscoe commented 2 years ago

@bradking and @mathstuf,

Now I understand this move of modern CMake to namespaced target names. From "Professional CMake: 10th edition"

Another important aspect of names having a double-colon (::) is that CMake will always treat them as the name of an alias or imported target. Any attempt to use such a name for a different target type will result in an error. Perhaps more usefully though, when the target name is used as part of a target_link_library() call, if CMake doesn’t know of a target by that name, it will issue an error at generation time. Compare this to an ordinary name which CMake will treat as a library assumed to be provided by the system if it doesn’t know of a target by that name. This can lead to the error only becoming apparent much later at build time.

That is what we are suffering from here. Old CMake used non-namespaced targets and if a target <some-lib> become undefined, target_link_libraries( ... <some-lib>) will just assume that <some-lib> is out on the system and put in a -l<some-lib> on the link line. That makes it impossible to catch errors in target names at configure or generation time. You only find out about errors at link time where it is very difficult to debug what is happening (unless you know to watch for this).

Given this information, I think that you should never link against a non-namespaced target in a call to target_link_libraries(). CMake just provides not checking whatsoever at configure or generation time with raw non-namespaced names. Someone should have a list of basic CMake dos and don'ts and this needs to be on that list (both as "do create and link against namespaced targets" and "don't link against non-namespaced targets").

bradking commented 2 years ago

you should never link against a non-namespaced target in a call to target_link_libraries()

FYI, CMake 3.23 adds an optional LINK_LIBRARIES_ONLY_TARGETS check to verify that plain names passed to target_link_libraries are actually targets.

bradking commented 2 years ago

create these ALIAS DEPRECATION non-namespaced targets directly in the generated Config.cmake file.

FYI, CMake 3.18 is required to create an ALIAS target that references a non-global imported target. ALIAS targets were not originally meant for this use case.

An alternative is to create a non-namespaced IMPORTED INTERFACE library that just sets INTERFACE_LINK_LIBRARIES to the namespaced target's name.

bradking commented 2 years ago

hoping that I could just get CMake to generate these targets automatically...I am not sure If CMake will let me do this.

I don't think it will. While one might be able to call install(EXPORT) twice, once with a namespace and once without, doing so would create two independent definitions of each target that don't de-duplicate and could double-up linking.

I think generating the compatibility targets yourself in TribitsProjectConfigTemplate.cmake.in or similar may work best.

bartlettroscoe commented 2 years ago

FYI, CMake 3.23 adds an optional LINK_LIBRARIES_ONLY_TARGETS check to verify that plain names passed to target_link_libraries are actually targets.

That is great news! Unfortunately, we can't turn this on for Trilinos right now due to people taking advantage of raw library names (see https://github.com/trilinos/Trilinos/pull/9978#issuecomment-982166363).

@bradking, would you recommend people change from passing raw <libname> to target_link_libraries() such as for:

 -D TPL_BLAS_LIBRARIES:PATH="${OPENBLAS_PATH}/lib/libopenblas.a;gfortran;gomp;m"

to passing -l<libname>:

 -D TPL_BLAS_LIBRARIES:PATH="${OPENBLAS_PATH}/lib/libopenblas.a;-lgfortran;-lgomp;-lm"

?

Is -l<libname> portable on Windows?

(I did not even realize that target_link_libraries() accepted raw lib names like this until https://github.com/trilinos/Trilinos/pull/9978#issuecomment-982166363 was posted. I had never used that in 13+ years of my CMake usage. And yes, I know people should be using find_packge() modules but that is a hard sell when people already have working configure scripts and people have really struggled with some of the Find<Package>.cmake modules over the years.)

One of the issues we are struggling with here is that early versions of CMake were incredibly permissive so attempts we make to improve things and move to modern CMake often result in errors that are not caught until compile or link time where they are very hard for some people to debug. But I guess everyone is in the same boat. (But Trilinos has a lot of customers that configure, build, and then link against Trilinos on all types of crazy systems by thousands of non-CMake users across the planet.)

bartlettroscoe commented 2 years ago

I think generating the compatibility targets yourself in TribitsProjectConfigTemplate.cmake.in or similar may work best.

@bradking. I can do that, but that means that I will need to add some machinery to keep a list of targets for which to create these INTERFACE IMPORTED targets when generating the <Package>Config.cmake file from TribitsPackageConfigTemplate.cmake.in. But I am already gathering that list of targets to form the::all_libs` target at:

https://github.com/TriBITSPub/TriBITS/blob/c8abb338d90d9f4065cd415eca01c7212a754084/tribits/core/package_arch/TribitsPackageMacros.cmake#L718-L729

I just need to pass that list packageLibsInAllLibsList to the generated file <Package>Config.cmake and let it loop over to form the non-namespaced INTERFACE IMORTED targets.

(NOTE: All of this code will get cleaned up significantly once we are done with this transition to full modern CMake inside of TriBITS. Getting rid of all of the variables and global properties will be one of the major goals.)

bradking commented 2 years ago

Raw library names are actually the preferred way to refer to toolchain-provided libraries, rather than find_library, so that the toolchain can handle selecting the proper library file from its builtin paths. LINK_LIBRARIES_ONLY_TARGETS documents a way to handle such libraries via IMPORTED_LIBNAME on an interface library, but only when the project code knows the names itself.

Is -l<libname> portable on Windows?

No. That would just pass through directly to the link line as a flag, and the MSVC linker does not understand such syntax.

Who is responsible for passing ${TPL_BLAS_LIBRARIES} to a target_link_libraries call? Is project code free to do that, or does TriBITS handle it somewhere? In the latter case, it might be able to transform the content.

bartlettroscoe commented 2 years ago

Who is responsible for passing ${TPL_BLAS_LIBRARIES} to a target_link_libraries call? Is project code free to do that, or does TriBITS handle it somewhere? In the latter case, it might be able to transform the content.

@bradking, that is a good point. The TriBITS refactoring that I just finished actually does process the list in ${TPL_BLAS_LIBRARIES} (all all the other TPLs) to create IMPORTED library targets and does not rely on passing raw library names to target_link_libraries(). So with the upgraded TriBITS, in theory, Trilinos could set LINK_LIBRARIES_ONLY_TARGETS=TRUE. I created the backlog item https://github.com/trilinos/Trilinos/issues/10081 for this.

bartgol commented 2 years ago

I am not reading the whole conversation, but I read that some changes won't happen due to downstream apps relying on "old" cmake patterns. FWIW, I wanted to make it clear that on the Albany side we are ok if Trilinos moves to moder cmake and breaks our builds. We will fix them up. Having more robust (and self-explanatory) cmake logic is definitely healthy, and I'm willing to take a few hours to adapt to whatever change you guys might do.

I know we're not the only/main trilinos customer, but I figured stating it clearly could help (especially if other apps are on the same page).

bartlettroscoe commented 2 years ago

Consensus at meeting today involving @bradking, @mathstuf, @mperrinel, @keitat: go with option-1 and add back non-namespaced targets and add a good DEPRECATION message that mentions the usage of ${<Package>_LIBRARIES} to build against old and new TriBITS/Trilinos or just use the updated TriBITS/Trilinos and use<Package>::all_libs and drop usage of the <Package>_INCLUDE_DIRS and <Package>_TPL_INCLUDE_DIRS variables.

bartlettroscoe commented 2 years ago

FWIW, I wanted to make it clear that on the Albany side we are ok if Trilinos moves to modern cmake and breaks our builds. We will fix them up. Having more robust (and self-explanatory) cmake logic is definitely healthy, and I'm willing to take a few hours to adapt to whatever change you guys might do.

@bartgol, it is good that some customers are willing to absorb non-backward compatible changes. Long term, this has to happen if we are going to move to more robust modern CMake as a community. This issue is how painful does this need to be for everyone involved? For the case of linking against the old non-namespaced targets, that resulted in a link-time error what was not trivial to debug (at least not for me). We want to provide a smoother path to get customers to upgrade their usage of Trilinos for modern CMake and encourage them to adopt modern CMake. If it was a clear configure-time error, then I think that is likely okay. But a obscure link time error is not okay (at least not in my opinion).