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

[magnum] Properly deploy plugins #3191

Merged
merged 7 commits into from Apr 19, 2018

Conversation

Squareys
Copy link
Contributor

@Squareys Squareys commented Apr 2, 2018

Hi @ras0219-msft !

I finally fixed the plugin deployment of the magnum package (see mosra/magnum#235)! As this is the first time I came in contact with Powershell, you may want to have a close look at that.

Also, to be able to build --head with upcoming features properly, I added the repective features and incremented the CONTROL-file version.

@mosra may want to have a quick glance at the depedencies of new features.

Thanks in advance,
Jonathan.

@Squareys
Copy link
Contributor Author

Squareys commented Apr 3, 2018

@mosra Done with the changes discussed on gitter! :)

Copy link

@mosra mosra left a comment

Choose a reason for hiding this comment

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

Thanks a lot. 👍

Tricky question: does this automagically copy also dependencies of the plugins? (e.g. is libpng DLL that PngImporter depends on copied as well?) Or does vcpkg handle that part already? :)

@@ -41,6 +43,7 @@ vcpkg_configure_cmake(
OPTIONS
${_COMPONENT_FLAGS}
-DBUILD_STATIC=${BUILD_STATIC}
-DBUILD_PLUGINS_STATIC=${DBUILD_PLUGINS_STATIC}
Copy link

Choose a reason for hiding this comment

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

Hmm, maybe just have a single BUILD_PLUGINS_STATIC variable and set it to both? Also, you have DBUILD there ;)

-DBUILD_STATIC=${BUILD_PLUGINS_STATIC}
-DBUILD_PLUGINS_STATIC=${BUILD_PLUGINS_STATIC}

Build-Depends: freeglut
Feature: anyaudioimporter
Description: (Upcoming) AnyAudioImporter plugin
Build-Depends: magnum[trade]
Copy link

Choose a reason for hiding this comment

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

magnum[audio] instead


Feature: glutapplication
Description: GlutApplication library
Build-Depends: freeglut
Copy link

Choose a reason for hiding this comment

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

Build-Depends: freeglut, magnum[gl]

Description: MeshTools library
Feature: opengltester
Description: OpenGLTester library
Build-Depends: corrade[testsuite]
Copy link

Choose a reason for hiding this comment

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

Build-Depends: corrade[testsuite], magnum[gl]

Description: (Upcoming) Trade library

Feature: wavaudioimporter
Description: WavAudioImporter plugin
Build-Depends: magnum[audio]

Feature: windowlesswglapplication
Description: WindowlessWglApplication library

Feature: wglcontext
Description: WglContext library
Copy link

Choose a reason for hiding this comment

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

Build-Depends: magnum[gl]


Feature: distancefieldconverter
Description: magnum-distancefieldconverter utility
Build-Depends: magnum[texturetools]

Feature: fontconverter
Description: magnum-fontconverter utility
Build-Depends: magnum[text]
Copy link

Choose a reason for hiding this comment

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

Build-Depends: magnum[text], magnum[gl]


Feature: distancefieldconverter
Description: magnum-distancefieldconverter utility
Build-Depends: magnum[texturetools]
Copy link

Choose a reason for hiding this comment

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

Build-Depends: magnum[texturetools], magnum[gl]

@@ -79,3 +87,6 @@ Feature: stbvorbisaudioimporter
Description: StbVorbisAudioImporter plugin
Build-Depends: magnum[audio]

Feature: tinygltfimporter
Description: (Upcoming) TinyGltfImporter plugin
Copy link

Choose a reason for hiding this comment

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

This is not upcoming anymore :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it's not in the release that has been tagged in the accompanying portfile.cmake, so "in perspective of the last release" it is indeed upcoming until that is tagged :)

Copy link

Choose a reason for hiding this comment

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

Ah, sure, true, then leave it there. Sorry, my perception of time is mixed up again :)

@@ -1,31 +1,35 @@
Source: magnum-plugins
Version: 2018.02-1
Build-Depends: magnum
Version: 2018.02-2
Copy link

Choose a reason for hiding this comment

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

Why no Build-Depends: magnum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the magnum[trade] and didn't think :D

resolve($targetBinary)
Write-Verbose $($g_searched | out-string)
Copy link

Choose a reason for hiding this comment

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

Why this was removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, thanks!

@Squareys
Copy link
Contributor Author

Squareys commented Apr 3, 2018

does this automagically copy also dependencies of the plugins? (e.g. is libpng DLL that PngImporter depends on copied as well?) Or does vcpkg handle that part already? :)

Not so tricky answer: Yes! :D

Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Prepares upcoming move of those sublibraries and allows building --head
immediately.
For current release this only adds some unused cmake flags that will be
ignored.

Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys
Copy link
Contributor Author

Squareys commented Apr 3, 2018

@mosra Done making the requested changes, thank you very much for the review!

@ras0219-msft This is now ready to merge, thanks for the help with mosra/magnum#235 !
PS: A while ago I opened this issue here, it would be great to get some brief feedback on that.

And sort features alphabetically.

Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys
Copy link
Contributor Author

Squareys commented Apr 5, 2018

@mosra Added the two missing feature dependencies, as discussed per gitter.

You may need to reapprove.

Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys
Copy link
Contributor Author

@ras0219-msft This is ready to be merged. (In case it wasn't clear)

@ras0219-msft
Copy link
Contributor

Thanks, this looks good to me as well!

@ras0219-msft ras0219-msft merged commit 3a3fa5c into microsoft:master Apr 19, 2018
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