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

Specify download mirror for libs #6834

Merged
merged 1 commit into from Jul 4, 2015

Conversation

Projects
None yet
7 participants
@jediry
Contributor

jediry commented Mar 29, 2015

This change improves project\BuildDependencies\DownloadBuildDeps.bat (and friends). Several times now, I've run into issues with the mirror selected from http://mirrors.xbmc.org being broken. I'm addressing this by adding support for a KODI_MIRROR environment variable; if set, the download scripts will use that mirror rather than auto-picking one from http://mirrorrs.xbmc.org. If DownloadBuildDeps.bat hits a failure downloading something, it will print out a message telling the user how to set KODI_MIRROR and try again.

I've also fixed a handful of other issues in the download scripts:

  1. They can now be run from directories other than project\BuildDependencies. Now, if you run DownloadBuildDeps.bat from the kodi root directory, it will no longer delete the checked in files in kodi\lib...instead, all paths are constructed relative to %~dp0 (the directory containing the script).
  2. get_formed.cmd now reports a list of failed downloads at the end, so if something failed, you can see what it was.
  3. Fix a few places where errors were not handled, or missing directories were not created.

@jediry jediry closed this Mar 29, 2015

@jediry jediry reopened this Mar 29, 2015

SET APP_PATH=%CD%\..\..
SET TMP_PATH=%CD%\scripts\tmp
:: If KODI_MIRROR is not set externally to this script, set it to the default mirror URL
IF "%KODI_MIRROR%" == "" SET KODI_MIRROR=http://mirrors.xbmc.org

This comment has been minimized.

@Memphiz

Memphiz Mar 29, 2015

Member

mirrors.kodi.tv please while you are at it

This comment has been minimized.

@jediry

jediry Mar 29, 2015

Contributor

Sure thing. Will fix in next commit.

@@ -29,14 +37,26 @@ md %TMP_PATH%
cd scripts
SET FORMED_OK_FLAG=%TMP_PATH%\got-all-formed-packages
REM Trick to preserve console title
:: Trick to preserve console title

This comment has been minimized.

@Memphiz

Memphiz Mar 29, 2015

Member

what is the benefit of :: instead of rem?

This comment has been minimized.

@jediry

jediry Mar 29, 2015

Contributor

They're equivalent, as far as cmd.exe is concerned. But I think it reads more naturally to a human if the characters that introduce a comment are not themselves an English word. I can switch them all to "rem" if someone feels strongly about it.

This comment has been minimized.

@war59312

war59312 Jun 8, 2015

Should use "REM" for comments. Using "::" for comments can cause issues.

See: http://www.robvanderwoude.com/comments.php

The double-colon is not documented as a comment command, it is a special case of a label that acts as a comment. Meaning, it is NOT supported and can change in new builds of windows. So why use it when REM is the correct method for comments.

And the "slow" issue mentioned above is not really an issue anymore. Perhaps back in Win95 days.

This comment has been minimized.

@jediry

jediry Jun 9, 2015

Contributor

Very well. Fixed in most recent commit.

ECHO.
ECHO I tried to get the packages from %KODI_MIRROR%;
ECHO if this download mirror seems to be having problems, try choosing another from
ECHO the list on http://mirrors.xbmc.org/list.html, and setting %%KODI_MIRROR%% to

This comment has been minimized.

@Memphiz

Memphiz Mar 29, 2015

Member

kodi.tv here too

This comment has been minimized.

@jediry

jediry Mar 29, 2015

Contributor

Ok

ECHO if this download mirror seems to be having problems, try choosing another from
ECHO the list on http://mirrors.xbmc.org/list.html, and setting %%KODI_MIRROR%% to
ECHO point to it, like so:
ECHO C:\^> SET KODI_MIRROR=http://example.com/pub/xbmc/

This comment has been minimized.

@Memphiz

Memphiz Mar 29, 2015

Member

irritating ^

This comment has been minimized.

@jediry

jediry Mar 29, 2015

Contributor

It's necessary. ^> is the escape sequence to print a literal ">". Without the ^, cmd.exe interprets the > as output redirection.

rem update fstab to install path
IF NOT EXIST %MSYS_INSTALL_PATH%\etc md %MSYS_INSTALL_PATH%\etc

This comment has been minimized.

@Memphiz

Memphiz Mar 29, 2015

Member

why was that not needed before?

This comment has been minimized.

@jediry

jediry Mar 29, 2015

Contributor

Good question...I'm not sure. It might be unnecessary now that I've fixed the "only works when run from BuildDependencies directory" issue. I'll try removing it and see if it all still works.

This comment has been minimized.

@jediry

jediry Mar 29, 2015

Contributor

This change does not appear to be necessary anymore. I'll revert it in the next commit.

@jediry

This comment has been minimized.

Contributor

jediry commented Mar 29, 2015

I have a git-related question: If you look at the commits associated with this pull request, you'll see that I inadvertently incorporated a commit (daccd62) from another outstanding pull request (it was in my "master" branch, so it was automatically incorporated into the "specify_download_mirror" branch when I created it). That didn't seem appropriate, since it's an unrelated change, so I did a "git revert" on that change in my topic branch. Now, I'm wondering if that was the right thing to do...what happens when both pull requests are merged into the main repo? Will my "revert" in this branch effectively cancel out the other pull request?

If this is going to be a problem when both pull requests are merged, can someone tell me how to fix this? I'm kind of a git newbie. (Intuitively, it seems like the solution might be to "rebase" this branch so that it no longer incorporates commit daccd62, but I don't understand git well enough to understand all the implications of rewriting the repro history, particularly since I've already pushed the current version of the specify_download_mirror branch from my local repro into my github fork.

@hudokkow

This comment has been minimized.

Member

hudokkow commented Mar 29, 2015

No implications and no problems since your fork isn't being used by anyone else.

Fastest way using shell:
git rebase -i HEAD~6
Your default editor will open. Delete the line referencing the commit you wish to throw away (daccd62).
Save the file. git will drop back to the shell and rewrite history.
git push -f
will update your branch and this PR.

More about rebase https://help.github.com/articles/about-git-rebase/#an-example-of-using-git-rebase and how useful it is ;)

@wsnipex

This comment has been minimized.

Member

wsnipex commented Mar 30, 2015

I like the idea. Sent you a PR that adds the same env var for all other platforms as well.

@wsnipex

This comment has been minimized.

Member

wsnipex commented Apr 2, 2015

Please rebase, squash fixup commits and remove the merge commits.

@jediry

This comment has been minimized.

Contributor

jediry commented Apr 5, 2015

wsnipex, could you clarify? You want me to squash everything as-is into a single commit, including your changes that I merged? Or are you you asking me to revert the merge, and squash everything else?

@wsnipex

This comment has been minimized.

Member

wsnipex commented Apr 5, 2015

1.) all "Merge branch/pull request" commits must be removed
2.) squash your fixup, revert and "incorporate feedback" to some of your commits. If you make them all a single commit is up to you, but I guess it'd make sense.
3.) keep mine as is

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented May 23, 2015

something went wrong on this as i can't see any changes anymore. Care to redo the changes and force push into a single commit to this branch?

@jediry jediry closed this May 31, 2015

@jediry

This comment has been minimized.

Contributor

jediry commented Jun 1, 2015

Yeaaaaaah...so, I screwed up my attempt to rebase the commits, and inadvertently obliterated them from the universe. I had an earlier version of the changed "git stash"-ed, and I'm trying to reconstruct the change as best I can. Sigh.

Should be done later this week.

@Montellese

This comment has been minimized.

Member

Montellese commented Jun 1, 2015

Take a look at git reflog. It will show you the changes that you did to your git repository and you can look for your rebase and simply git reset --hard HEAD@{X} to the reflog entry before your rebase.

@jediry jediry reopened this Jun 7, 2015

@jediry jediry closed this Jun 7, 2015

@jediry

This comment has been minimized.

Contributor

jediry commented Jun 7, 2015

OK! I think I've fixed things. I also found a few other uses of mirrors.xbmc.org and converted them to use %KODI_MIRROR% where I could figure out how to do that, and to http://mirrors.kodi.tv if I couldn't.

I'm leaving the PR closed for the moment, as I need to verify my changes didn't break the Linux build, and I will reopen once I've confirmed that.

@hudokkow

This comment has been minimized.

Member

hudokkow commented Jun 7, 2015

No need to close/open the PR all the time. No one will merge/close/whatever until reviewed, tested and agreed code is ok.

@hudokkow hudokkow reopened this Jun 7, 2015

@jediry

This comment has been minimized.

Contributor

jediry commented Jun 7, 2015

OK, good to know.

I did a test bootstrap/configure/make of Kodi on Linux, and it all appeared to build fine. I'm not sure how to test the change to tools/rbp/setup-sdk.sh. I'm guessing that's for Raspberri Pi.

@wsnipex

This comment has been minimized.

Member

wsnipex commented Jun 8, 2015

jenkins build this please

ifneq ($(KODI_MIRROR),)
KODI_MIRROR=http://mirrors.kodi.tv
endif
BASE_URL=$(KODI_MIRROR)/build-deps/sources

This comment has been minimized.

@wsnipex

wsnipex Jun 8, 2015

Member

to fix OSX, please change that block to

BASE_URL=http://mirrors.kodi.tv/build-deps/sources
ifneq ($(KODI_MIRROR),)
BASE_URL=$(KODI_MIRROR)/build-deps/sources
endif

@jediry

This comment has been minimized.

Contributor

jediry commented Jun 8, 2015

Fixed

Improve Win32 build/download scripts. This change adds support for
a KODI_MIRROR environment variable which, if set, overrides the
default mirror URL of http://mirrors.kodi.tv. In the event that a
package download fails, DownloadBuildDeps.bat will now suggest that
the user manually choose an alternate mirror and re-run.

Also, this change improves the download scripts in several ways.
get_formed.cmd now reports failed package downloads at the end,
and DownloadBuildDeps.bat, DownloadMingwBuildEnv.bat and
make-mingwlibs.bat all support being run from an arbitrary directory
(e.g., c:\kodi> project\BuildDependencies\DownloadBuildDeps.bat).
@wsnipex

This comment has been minimized.

Member

wsnipex commented Jun 9, 2015

jenkins build this please

@wsnipex

This comment has been minimized.

Member

wsnipex commented Jun 9, 2015

@Montellese your button

@Montellese

This comment has been minimized.

Member

Montellese commented Jun 10, 2015

@wsnipex: Why me? :-P Changes to win32 look ok. Should we wait till Isengard has been branched as this is not really a fix to Kodi itself? I'd rather not break the buildsystem right now.

@Montellese Montellese added the Fix label Jun 10, 2015

@MartijnKaijser

This comment has been minimized.

Member

MartijnKaijser commented Jun 10, 2015

we would need to make sure it would fetch all changes starting from an empty workspace. Let's put this up for next version and we'll do a workspace wipe before building

Edit:
we could then tell jenkins to always pull from closed mirror instead of going through our normal mirror system. Would prevent build fails in case of broken mirror system

@MartijnKaijser MartijnKaijser added this to the Isengard 16.0-alpha1 milestone Jul 3, 2015

@MartijnKaijser MartijnKaijser changed the title from Specify download mirror for win32 download batch files to Specify download mirror for libs Jul 4, 2015

MartijnKaijser added a commit that referenced this pull request Jul 4, 2015

@MartijnKaijser MartijnKaijser merged commit 10ed258 into xbmc:master Jul 4, 2015

1 check passed

default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment