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

[cmake] Crossguid offline url override #10948

Merged
merged 1 commit into from Nov 18, 2016

Conversation

@BrandonSchaefer
Copy link

commented Nov 16, 2016

Crossguid offline url override

Description

When attempting to build offline crossguid will still attempt to download even when you override the URL. It also does not generate the full path for you. Updated it so it will override the URL the same way as ffmpeg and libdvd.

I could also be assuming something here but trying to use it like ffmpeg/libdvd. If so just reject this PR!

Motivation and Context

Would like to override the URL for offline building.

How Has This Been Tested?

Test by -DCROSSGUID_URL=foo.tar.bz2 then make crossguid and see that it still downloads the tar ball. With the change, it wont download anymore! (Though it'll fail unless you have a foo.tar.bz2 as the real crossguild tar ball!)

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
@BrandonSchaefer

This comment has been minimized.

Copy link
Author

commented Nov 16, 2016

More or less the issue is, if you provide a non full path it'll fail and download. Since ffmpeg and libdvd generate the fullpath for you it would be more consistent to do it how the others are doing it.

@hudokkow

This comment has been minimized.

Copy link
Member

commented Nov 16, 2016

Sorry, it's almost AM here and I'm not sure I understand the problem. What do you mean by a non full path? Are you only passing the filename?

Though it'll fail unless you have a foo.tar.bz2 as the real crossguild tar ball!

Again, don't fully understand what you mean but I presume you want to check if files exists:

if(CROSSGUID_URL AND EXISTS CROSSGUID_URL)
[...]

Thanks for taking the time to look into this. CMake is very much a work in progress and all input is welcome. Ideally we should cache downloaded files and look in there first. Not quite there yet. 😉

@hudokkow hudokkow added the CMake label Nov 16, 2016
@BrandonSchaefer

This comment has been minimized.

Copy link
Author

commented Nov 16, 2016

Non-full path meaning a non absolute path (relative path). So where ever your root CMakeLists.txt file is

For example:
-DCROSSGUILD_URL=downloads/crossguid-tar-ball.tar.bz2 -DFFMPEG_URL=downloads/ffmpeg-tar-ball.tar.bz2

Crossguild url fails, and ffmpeg url works.

Could be some cached download which for the absolute path would be:
/path/to/kodi/project/cmake/downloads/crossguid-tar-ball.tar.bz2

This issue is:
if (NOT EXISTS ) requires the absolute path, so giving it:
-DCROSSGUILD_URL=downloads/crossguid-tar-ball.tar.bz2

Will fail since that file does not exists, which means ill need to know the full absolute path from the command line. So it would be better if we did it like ffmpeg and libdvd (in FindFFMPEG.cmake) you give a relative path, which then cmake builds the absolute path with:
get_filename_component(FFMPEG_URL "${FFMPEG_URL}" ABSOLUTE)

Which is what Ive done here. So now you can use a relative path based on the root CMakeLists.txt vs knowing an absolute path from the command line

@BrandonSchaefer

This comment has been minimized.

Copy link
Author

commented Nov 16, 2016

Also why do we need to check if the file exists to fall back? The override is usually manually done, soo if they give an incorrect path, error out. That seems fair to me (otherwise it might be confusing if they are not paying attention and then use the downloaded version vs the one they are attempting to override with)

@hudokkow

This comment has been minimized.

Copy link
Member

commented Nov 16, 2016

Now I understand! Like I said, almost (very) AM. Sorry for wasting your time.

jenkins build this please

@BrandonSchaefer

This comment has been minimized.

Copy link
Author

commented Nov 16, 2016

O no worries i didnt explain it very well to begin with :)

@wsnipex

This comment has been minimized.

Copy link
Member

commented Nov 17, 2016

this is ok, thanks.

@@ -7,7 +7,9 @@ if(ENABLE_INTERNAL_CROSSGUID)

# allow user to override the download URL with a local tarball
# needed for offline build envs
if(NOT EXISTS ${CROSSGUID_URL})
if(CROSSGUID_URL)
get_filename_component(CROSSGUID_URL "${CROSSGUID_URL}" ABSOLUTE)

This comment has been minimized.

Copy link
@wsnipex

wsnipex Nov 17, 2016

Member

please add:
if(VERBOSE)
message(STATUS "CROSSGUID_URL: ${CROSSGUID_URL}")
endif()

@BrandonSchaefer BrandonSchaefer force-pushed the BrandonSchaefer:crossguid-offline branch from adebe2c to 4b6e23e Nov 17, 2016
@BrandonSchaefer

This comment has been minimized.

Copy link
Author

commented Nov 17, 2016

Done and squashed

@wsnipex wsnipex merged commit 46b4be8 into xbmc:master Nov 18, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@hudokkow hudokkow added this to the Krypton 17.0-beta6 milestone Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.