Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a Boost-friendly subproject case to CMakeLists #614

Merged
merged 1 commit into from
Oct 30, 2021
Merged

Conversation

pdimov
Copy link
Member

@pdimov pdimov commented Jun 4, 2021

No description provided.

@sdebionne
Copy link
Contributor

Shall we unify the subproject and standalone configurations (e.g project name boost_gil vs Boost.GIL) and add options / dependencies for the io extensions in the subproject configuration? Or it's OK if they are decorrelated?

@pdimov
Copy link
Member Author

pdimov commented Jun 15, 2021

You can unify them if you like, but it's not required (or urgent.)

@pdimov
Copy link
Member Author

pdimov commented Sep 16, 2021

Can this please be merged (incl. to master), so that I can remove gil from BOOST_INCOMPATIBLE_LIBRARIES?

@lpranam
Copy link
Member

lpranam commented Sep 16, 2021

obviously we can get it in . if not much to ask can we keep license on top of the file or is there any reason to keep the new configuration at top?

@pdimov
Copy link
Member Author

pdimov commented Sep 16, 2021

Feel free to reorder the comments in any way you like. :-)

@lpranam lpranam force-pushed the pr/cmakelists branch 2 times, most recently from 0bf2b29 to 168dbf1 Compare September 16, 2021 16:35
@mloskot
Copy link
Member

mloskot commented Sep 17, 2021

Clearly, I've failed to take care of this. Thanks @lpranam & @pdimov

@lpranam Please, merge when you think it's ready.

@mloskot mloskot added this to the Boost 1.77+ milestone Sep 17, 2021
@mloskot mloskot added the config/cmake Any issues about CMake configuration label Sep 17, 2021
@pdimov
Copy link
Member Author

pdimov commented Sep 17, 2021

The comment at the top

# **WARNING:**
# The CMake configuration is only provided for convenience
# of contributors. It does not export or install any targets,
# deploy config files or support subproject workflow.

is no longer strictly accurate, so you might either rephrase it or move it after the else().

@simmplecoder
Copy link
Contributor

I believe in the current state the library will not be usable due to missing IO dependency libraries and some builds indicate that internal boost dependencies might be missing too (align library is not in the target link libraries list but is used as include).

@pdimov could you please point to a place where the expected behavior of compatible libraries is documented? Should they just provide the new target names? In that case I believe it would be better to carefully insert the new target declarations instead of doing if/else. I will fix the builds as well (probably have to gut the CI infrastructure, hope you don't mind @mloskot ).

@pdimov
Copy link
Member Author

pdimov commented Sep 18, 2021

I believe in the current state the library will not be usable due to missing IO dependency libraries and some builds indicate that internal boost dependencies might be missing too (align library is not in the target link libraries list but is used as include).

boostdep --cmake gil says that the dependency list hasn't changed since I made this PR.

could you please point to a place where the expected behavior of compatible libraries is documented?

Not at the moment, sorry. But the behavior basically needs to be what boostdep --cmake gil says.

@pdimov
Copy link
Member Author

pdimov commented Sep 18, 2021

What this PR does is allow gil to be removed from BOOST_INCOMPATIBLE_LIBRARIES here:
https://github.com/boostorg/cmake/blob/4dfbec1d500598629d56e7f6b9057ec6d1115a11/include/BoostRoot.cmake#L22-L25
which would allow it to at least be installed using CMake. It also allows its header-only use in cases the io extensions aren't needed.

Doing this "properly" will be more involved. It will be easier, and more "regular", if gil_io is made a normal compiled library, instead of a header-only one, similar to Iostreams. This would allow it (at least in the shared lib case) to be used without needing libjpeg-devel et al.

The CMakeLists file of Boost.Iostreams is a good reference example for a similar library: https://github.com/boostorg/iostreams/blob/9edd46fe730e2086675a91629287224bd92590ab/CMakeLists.txt

But in either case, the general outline of boostdep --cmake gil needs to be followed, because any deviations are usually wrong, in various creative ways. For instance, isolating dependencies or compile options in INTERFACE subtargets, as currently done, breaks installation because these subtargets then need to be installed.

At the moment, what I have as documentation is here: https://github.com/boostorg/cmake/blob/doc/developer/developer.md. It's still on a branch because it's not yet finished, but the parts that are there, are accurate. I hope it's of help.

@simmplecoder
Copy link
Contributor

simmplecoder commented Sep 19, 2021 via email

@simmplecoder
Copy link
Contributor

@pdimov I read through the documents and peeked at sketches of cmake infrastructure. I do not think it is possible to make IO compiled library as they are mostly templated. What do you think about this: I will duplicate the Boost.IOStreams options function ( in the CMakeLists.txt the file you linked) and if the library is not found, there will be a preprocessor directive akin to the following (this example is for JPEG include files):

#ifndef BOOST_GIL_JPEG_FOUND
#warning "JPEG library was not found and was not linked, including this header is erroneous"
#endif

Each IO extension header will contain the preprocessor directives and if dependency was not found, any inclusion will cause a warning. I'm not sure if it is worth escalating the #warning into #error as since it is a template, as long as it is not instantiated it will not break anything (@mloskot @sdebionne @lpranam what do you think?). This should allow us some intermediate mode while we get clearer solution sorted out. I can do this until the end of the next week if we choose to go this route.

What Peter wanted was probably to make unusable headers unavailable, but due to nested structure and templates I do not think it is possible. In case of IOStreams it seems like there a common interface and the libraries are just backends (I guess they use virtual functions just like the standard streams), while in case of GIL it seems like at least major refactoring is needed in order to separate them (theoretically it should be possible to separate, but I do not have enough knowledge of the code base to say for sure).

Anyway, I do not think any immediate action will achieve anything useful. If nobody else has anything to add, I guess we could merge this as is for now.

@pdimov
Copy link
Member Author

pdimov commented Sep 22, 2021

and if the library is not found, there will be a preprocessor directive akin to the following

Or you can just do nothing and let the user deal with linking to the library; the preprocessor warning isn't particularly useful anyway.

@mloskot
Copy link
Member

mloskot commented Oct 30, 2021

I'm going to merge this into develop and, likely, cherry pick to master.

If this breaks thr old way of CMake for GIL, I'll fix it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config/cmake Any issues about CMake configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants