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

Download Intercept library to vendor folder on build if needed #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Dahlgren
Copy link
Contributor

No description provided.

@jonpas
Copy link
Member

jonpas commented Apr 19, 2018

It's an intercept plugin, of course it'll need intercept library?

@Dahlgren
Copy link
Contributor Author

Dahlgren commented Apr 19, 2018

Yes, using this feature of CMake it will automatically download Intercept when building if not already available. It removes the need to have git installed (when downloading the template as an archive) or knowledge about how git submodules work. Not all clients will automatically initialize submodules which can be confusing to users.

A new version of Intercept will also be automatically downloaded if version number if changed in the CMake file

@Dahlgren Dahlgren force-pushed the feature/cmake-external-project-intercept branch from a111180 to a5d7f41 Compare April 20, 2018 06:45
@dedmen
Copy link
Member

dedmen commented Apr 20, 2018

Can you also change
https://github.com/intercept/intercept-plugin-template/pull/2/files#diff-af3b638bc2a3e6c650974192a53c7291R1
to require version 3.8?
Just tried building with CMake 3.7.2 on debian and it fails because it doesn't know CXX17. Which gets added in CMake 3.8
I don't want to make a seperate commit for that 😀

Target "template-plugin_x64" requires the language dialect "CXX17" , but
CMake does not know the compile flags to use to enable it.

@Dahlgren
Copy link
Contributor Author

Sure, I'll fix it @dedmen

@Dahlgren Dahlgren force-pushed the feature/cmake-external-project-intercept branch from a5d7f41 to 3d7e2fa Compare April 20, 2018 10:44
@dedmen
Copy link
Member

dedmen commented Apr 20, 2018

:D trying to build on linux right now.. Guess what.. It didn't clone the submodule and thus the build fails.. duh.
With your PR added.

-- Downloading...
   dst='/home/wolf/projects/intercept-plugin-template/vendor/intercept/src/f283e2fd3ce1024e496c6c17eb27f81693f05ca8.tar.gz'
   timeout='none'
-- Using src='https://github.com/intercept/intercept/archive/f283e2fd3ce1024e496c6c17eb27f81693f05ca8.tar.gz'
-- Downloading... done
-- extracting...
     src='/home/wolf/projects/intercept-plugin-template/vendor/intercept/src/f283e2fd3ce1024e496c6c17eb27f81693f05ca8.tar.gz'
     dst='/home/wolf/projects/intercept-plugin-template/vendor/intercept/src/intercept'

get's the libs and builds just fi........ waaait..

[ 70%] Performing install step for 'intercept'
make[3]: *** No rule to make target 'install'.  Stop.
CMakeFiles/intercept.dir/build.make:73: recipe for target 'vendor/intercept/src/intercept-stamp/intercept-install' failed
make[2]: *** [vendor/intercept/src/intercept-stamp/intercept-install] Error 2
CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/intercept.dir/all' failed
make[1]: *** [CMakeFiles/intercept.dir/all] Error 2
Makefile:83: recipe for target 'all' failed
make: *** [all] Error 2

Where did it get that install step from. There is no install step.

intercept core doesn't have a install step. This apparently has for some reason. Do you know how to disable it?
I found
https://cmake.org/pipermail/cmake/2012-April/050023.html

# overwrite install() command with a dummy macro that is a nop
macro (install)
endmacro ()

Google tells me the ExternalProject adds that.

Ahh
https://stackoverflow.com/questions/37838786/how-to-not-make-install-step-when-building-external-project-with-cmake/37855224

Add

  STEP_TARGETS build
  EXCLUDE_FROM_ALL TRUE

to the External project.

Didn't even notice before. But I see that the plugin is also compiling the intercept host binaries. Which ofcause isn't needed. This disables that. too.

@dedmen
Copy link
Member

dedmen commented Apr 20, 2018

Okey that didn't work.
There is

    BUILD_COMMAND ""
    INSTALL_COMMAND ""

to disable the build and install steps. But that causes the plugin template to fail to find the intercept include which...
Makes total sense https://github.com/Dahlgren/intercept-plugin-template/blob/3d7e2faa819151bc3934512111d2270494b0fe0a/src/CMakeLists.txt#L19
it expects it in /intercept.
but with this it is in /vendor/intercept/src/intercept
I honestly would prefer it to stay in /intercept but I don't think ExternalProject can do that. Unless you make a install step for the project that moves the files.

Also hardcoded commit hash is a bad idea. Tons of people will grab this as zip. And then later on compile their plugins with a completly outdated intercept version while they don't know they have to update the hash.
But you can just enter master branch instead of hash right?
And I asked that before if it automatically downloads new master branch if there is one available and you kinda answered that here with

A new version of Intercept will also be automatically downloaded if version number if changed in the CMake file

The actual file link changing causing a new download is rather clear. But what about the branch download?

@Dahlgren
Copy link
Contributor Author

I'll give it a go on Windows tonight and resolve any issues. I'm unable to get it to even start building with clang on macOS but the CMake parts works for me.

@Dahlgren Dahlgren force-pushed the feature/cmake-external-project-intercept branch 3 times, most recently from 6119365 to cf27665 Compare April 20, 2018 19:57
@Dahlgren Dahlgren force-pushed the feature/cmake-external-project-intercept branch 5 times, most recently from cff9bef to 744f8cd Compare September 9, 2018 11:06
@Dahlgren Dahlgren force-pushed the feature/cmake-external-project-intercept branch from 744f8cd to 9ddbe9d Compare September 9, 2018 11:28
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

Successfully merging this pull request may close these issues.

None yet

3 participants