bpo-30450: Pull Windows dependencies from GitHub rather than svn by zware · Pull Request #1783 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation20 Commits18 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This is not an ideal solution, but may be enough to get us away from svn.python.org. The dependencies have not actually been pushed to python/cpython-source-deps or python/cpython-bin-deps yet, but you can use --organization zware for testing until we're agreed on the layout of everything.

@zware, thanks for your PR! By analyzing the history of the files in this pull request, we identified @methane and @ned-deily to be potential reviewers.

Choose a reason for hiding this comment

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

Looks good, though incomplete right now. And obviously AppVeyor is going to block until it's all fleshed out.

os.path.abspath(__file__) # '-> here
)
),
'externals'))

Choose a reason for hiding this comment

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

You're using f-strings but not pathlib? Shame ;)

verbose=args.verbose,
)
final_name = os.path.join(args.externals_dir, args.tag)
os.rename(extract_zip(args.externals_dir, zip_path), final_name)

Choose a reason for hiding this comment

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

Since we don't clean up the existing directory, should we fail early if it already exists? That would save downloading the zip just to fail at this point.

Choose a reason for hiding this comment

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

The other option is to either move the existing directory out of the way, or just overwrite it. I'm fine with any of those three options; opinions?

)

if %DO_FETCH%==false goto end

Choose a reason for hiding this comment

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

Good practice to use quotes around each side of the == in case one evaluates to empty.

Choose a reason for hiding this comment

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

Good point, will fix.

)
echo Installing Python via nuget...
%NUGET% install python -OutputDirectory %EXTERNALS_DIR%python
set PYTHON_FOR_BUILD=%EXTERNALS_DIR%python\python.3.6.1\tools\python.exe

Choose a reason for hiding this comment

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

If you use -ExcludeVersion with nuget and remove the extra python then the path should reliably be %EXTERNALS_DIR%python\tools\python.exe.

Choose a reason for hiding this comment

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

Alternatively, specify -Version 3.6.1 when installing to ensure that the path doesn't change unexpectedly.

Choose a reason for hiding this comment

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

Hadn't seen -ExcludeVersion; went with it. Just a little bit less hard-coding of variables.

powershell.exe -Command Invoke-WebRequest %NUGET_URL% -OutFile %NUGET%
)
echo Installing Python via nuget...
%NUGET% install python -OutputDirectory %EXTERNALS_DIR%python

Choose a reason for hiding this comment

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

Maybe we should actually install pythonx86 for both of our users building on 32-bit Windows? :)

Choose a reason for hiding this comment

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

Yep :)


def fetch_zip(commit_hash, zip_dir, *, org='python', binary=False, verbose):
repo = f'cpython-{"bin" if binary else "source"}-deps'
url = f'https://github.com/{org}/{repo}/archive/{commit\_hash}.zip'

Choose a reason for hiding this comment

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

This will download the entire repo at this commit - is your plan to use a branch per project? That's fine by me, just can't see the full picture yet.

Choose a reason for hiding this comment

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

Yes, that's the current layout. I don't think there's a way to get just a specific directory, and I like being able to not get OpenSSL or Tcl/Tk as desired.

@zooba If the scheme in my source and bin repos looks good enough to you, I'll go ahead and to python/* for easier (and AppVeyor) testing.

@zooba I did go ahead and push to the real repos, which makes AppVeyor happy. I've also successfully run Tools/msi/buildrealease.bat --test <dir> (a first).

Also fixes an outdated note about _lzma

@zware Paths with spaces in them are fine, but you'll need to be including quotes basically everywhere you use the variable. For build paths, I'd suggest leaving quotes out of the variable and using them when used, but for tools (e.g. the path to python.exe) I'd put the paths in the variable (so that py -3.6 is also a valid value and does not get quoted when invoked).

Essentially the same quoting rules apply to PowerShell as well.

Right, I eventually stopped messing with it on here and started working on it in my VM, got it mostly fixed, and then ran out of time before pushing and haven't gotten back to it...

Finally got this updated.

echo.Fetching external binaries...

set binaries=
set binaries=%binaries% binutils

Choose a reason for hiding this comment

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

I feel like this deserves a comment saying that we always build with the latest tools and so the versions are not part of the CPython sdist, unlike source dependencies. That will save me having to figure out why we don't have versions marked on these every time :)

Maybe using "tools" as the term rather than "binaries" would make more sense?

zware deleted the goodbye_svn branch

June 16, 2017 03:09

zware added a commit to zware/cpython that referenced this pull request

Jun 16, 2017

…honGH-1783)

The Windows build now depends on Python 3.6 to fetch externals, but it will be downloaded via NuGet (which is downloaded via PowerShell) if it is not available via py -3.6. This means the only thing that must be installed on a modern Windows box to do a full build of CPython with all extensions is Visual Studio.

Also fixes an outdated note about _lzma in PCbuild/readme.txt

zware added a commit that referenced this pull request

Jun 16, 2017

…1783) (GH-2237)

Also fixes an outdated note about _lzma in PCbuild/readme.txt

(cherry-picked from commit 51599e2)

zware added a commit to zware/cpython that referenced this pull request

Sep 4, 2017

…honGH-1783)

(cherry-picked from 51599e2)

zware added a commit that referenced this pull request

Sep 4, 2017

GH-1783) (GH-3306)

Cherry-picked from 51599e2, parts of 40a23e8, parts of 68d663c, d5cd21d, and possibly others that I've missed.

Also:

Rename db -> bsddb for disambiguity
Update sqlite3 to 3.14.2.0 since it's the version we use on 3.x, and it's simpler to just use it than to also upload the old version to cpython-source-deps
Add PCbuild/*.ilk to .gitignore