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

Make project compatible with FetchContent #32

Open
4 tasks
LecrisUT opened this issue Jun 9, 2023 · 11 comments
Open
4 tasks

Make project compatible with FetchContent #32

LecrisUT opened this issue Jun 9, 2023 · 11 comments

Comments

@LecrisUT
Copy link
Contributor

LecrisUT commented Jun 9, 2023

There are a few issues that should be resolved to make importing find_package and FetchContent equivalent:

  • alias library: fdict -> fdict::fdict
  • namespace targets, options, etc. In order to avoid nameclashes
  • Add options: FDICT_INSTALL, FDICT_SHARED_LIBS to control how fdict is being built
  • Set and propagate the variables in fdictConfig.cmake using return(PROPAGATE) or set(PARENT_SCOPE)
@zerothi
Copy link
Owner

zerothi commented Jun 9, 2023

Perhaps you can clarify some details I have been having about cmake, in the Config.cmake files generated, should one always specify set({pkg}_FOUND TRUE) or not? For instance you might end up in situations where a configuration file creates successfully finds the target it is supposed too, but it doesn't return _FOUND=true, the cmake documentation regarding this is very poor, and I cannot seem to figure out conventions in other packages?

Lastly, I have been focusing on getting the cmake stuff in. And was planning on getting the sub-project part done soonish, so I'll get my hands dirty again and will likely ask you some questions (here) if you don't mind? :)

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Jun 9, 2023

should one always specify set({pkg}_FOUND TRUE) or not?

In principle one does not have to do it. Normally you set <Package>_FOUND=FALSE when you specify that the import fails. I need to do more testing to confirm why <Package>_FOUND is not set to TRUE.

And was planning on getting the sub-project part done soonish, so I'll get my hands dirty again and will likely ask you some questions (here) if you don't mind? :)

Sure thing, feel free to make a PR and I can review and give some tips there.

@zerothi
Copy link
Owner

zerothi commented Jun 9, 2023

should one always specify set({pkg}_FOUND TRUE) or not?

In principle one does not have to do it. Normally you set <Package>_FOUND=FALSE when you specify that the import fails. I need to do more testing to confirm why <Package>_FOUND is not set to TRUE.

That information would be great, I have just encountered problems where the targets were found, but the _FOUND was not set (hence false), and then a Warning is issued for the users. This is then wrong... My solution was to never trust _FOUND but always check for the target...

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Jun 9, 2023

That information would be great, I have just encountered problems where the targets were found, but the _FOUND was not set (hence false), and then a Warning is issued for the users.

Note that FetchContent does not do an install() + find_package(), and in that case you need to synchronize the variables set in find_package() (hence the issue raised here)

@zerothi
Copy link
Owner

zerothi commented Jun 10, 2023

Might I ask what you are planning to use fdict for?

@LecrisUT
Copy link
Contributor Author

Might I ask what you are planning to use fdict for?

I don't know if it's a well-known design pattern, but I'm using this to add a context object/field so it is easier to write extensions in octopus. I.e. add an arbitrarily typed dictionary which is exactly what this package is doing

@zerothi
Copy link
Owner

zerothi commented Sep 18, 2024

I believe this is now fixed. Please re-open if necessary!

@zerothi zerothi closed this as completed Sep 18, 2024
@zerothi zerothi reopened this Sep 18, 2024
@zerothi
Copy link
Owner

zerothi commented Sep 18, 2024

Well, actually a small question @LecrisUT.

I removed FDICT_INSTALL because I ended up in problems where the targets where not exported correctly if I couldn't install them... So what do you do when install is hidden behind FDICT_INSTALL when you want it to be used as a subproject? I couldn't get it to work!

@LecrisUT
Copy link
Contributor Author

I removed FDICT_INSTALL because I ended up in problems where the targets where not exported correctly if I couldn't install them

Not sure I follow that. With export you mean the FetchContent consuming project would not be able to install correctly? Do you have a version to look at?

@zerothi
Copy link
Owner

zerothi commented Sep 18, 2024

I mean something like this:

if(FDICT_INSTALL)
  install(... )
  export(... NAMESPACE ...)
endif()

In these cases the target will not be exported, which is kind of what I need? No?

If I add it as a subdirectory, it complained about the target not defined. And I would rather use the export functionality to expose the namespace target, rather than manually defining it (seems redundant!)?

@LecrisUT
Copy link
Contributor Author

In these cases the target will not be exported, which is kind of what I need? No?

Not really, if it's only used as a static library for a binary having just the binary installed should be fine. I remember there being some weirdness when a shared library target links to a static and it's then exported. I don't remember what was going on there though.

export() only exports targets for build-dir, and install(EXPORT) only at install step.

If I add it as a subdirectory, it complained about the target not defined. And I would rather use the export functionality to expose the namespace target, rather than manually defining it (seems redundant!)?

For add_subdirectory you should use add_library(ALIAS). In order to use export you need to find_package and that's quite a complicated procedure.

Is it complaining when trying to install a fdict_user project?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants