Skip to content
This repository was archived by the owner on Jul 8, 2022. It is now read-only.

Conversation

@ltjax
Copy link

@ltjax ltjax commented Dec 17, 2019

Sorry for the PR bombing, this was all needed to get this to compile with conan!

Previously, PTHREAD_WIN was optional for building, but not for installing. Not it is. I have also unified the way that the variable is read - now: only through CMake, and no longer as an environment variable. Appveyor should already be set up correctly to handle that.

Also, why does Tango handle installation of its dependencies? Shouldn't that be the job of those (or, at least, a different CMake script?)

@bourtemb
Copy link
Member

Sorry for the PR bombing, this was all needed to get this to compile with conan!

Previously, PTHREAD_WIN was optional for building, but not for installing. Not it is. I have also unified the way that the variable is read - now: only through CMake, and no longer as an environment variable. Appveyor should already be set up correctly to handle that.

I think it's a valid point. I think we should probably stop using environment variables everywhere and use CMake definitions instead if technically possible.
@NexeyaSGara , do you remember why it was done like that? Or is it simply that we managed to make it work that way at some point and then nobody took the time to improve it? I think there were some issues when not using $ENV but I don't remember what kind of issues.
Do you think we could remove all these $ENV{MY_VAR} in cmake-win.cmake for instance and simply replace them with ${MY_VAR} as done in this PR for PTHREAD_WIN?

Also, why does Tango handle installation of its dependencies? Shouldn't that be the job of those (or, at least, a different CMake script?)

Good question. On Linux, we don't handle the installation of the dependencies, indeed.
I guess this is like that for historical reasons.
The CMake compilation on Windows is relatively recent and the CMakefiles were written with the goal to be able to take advantage of appveyor to compile the library and provide artifacts and to generate some installers so the Tango users would not have to worry about compiling cppTango on Windows any longer. This is now done automagically and the dependencies are provided by the generated installer. Since appveyor seems to be working fine and nobody got the need to compile cppTango on Windows by itself, the files were not touched much after the first successes with the appveyor compilation.
But of course, this could be improved.

Copy link
Member

@bourtemb bourtemb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.
But let's wait for the feedback from @NexeyaSGara who knows this part of the project better before to merge it.

Copy link
Collaborator

@t-b t-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@NexeyaSGara
Copy link
Collaborator

Hi,

I did it this way by lack of CMake knowledge. So I'm completly for this kind of improvements.

Copy link
Collaborator

@NexeyaSGara NexeyaSGara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@t-b t-b merged commit aa9780e into tango-controls:tango-9-lts Jan 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants