download .tar.xz if python3 is used

[guanqun]

fixes #65757

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[alexcrichton]

Thanks! Is there something that we should test for in Python to see if xz decompression is supported? Is it possible to configure that off in python 3? Are we sure that all Python 3 versions support this?

[alex]

Off hand, I'm almost certain it's possible to compile without it, and you can check for availability by seeing if import lzma raises an ImportError.

[guanqun]

Good catch, I added an extra check for the version, according to the python documentation here: https://docs.python.org/3/library/tarfile.html

Changed in version 3.3: Added support for lzma compression.

If we trust this info, checking the version is enough, we don't have to try to import lzma.

[tesuji]

We could use docker run --rm -it python:3.3 and test xz support in there.

[Mark-Simulacrum]

Can we just check for lzma support directly, and not do any version detection? That seems more reliable and avoids these questions.

[tesuji]

Maybe:

tarball_suffix = '.tar.xz'
try:
    import lzma
except ImportError:
    tarball_suffix = '.tar.gz'

[guanqun]

updated to import check, please help review again.

btw how can we update the azure pipelines to do a full verification?

[alexcrichton]

Is it certain that if lzma exists as a module that the tarball extraction module supports it as a decompression format?

This seems like it's getting somewhat complicated, so I'm not sure if it's worth having so many checks to avoid a few megabytes on the download? (presumably the git repo is much larger than the snapshots at this point?)

[guanqun]

According to the tarfile's implementation from python, it internally uses lzma, see the source code here: https://github.com/python/cpython/blob/master/Lib/tarfile.py#L387-L397

but in python2, even we have lzma, I doubt it can work with tarfile out of box.

So that's why I originally think it's better to do the version check, it's simple and it can always work though it might be a more conservative estimate.

[alexcrichton]

That's what I'm sort of getting at with maybe not doing this at all because it doesn't save all that much and it's pretty complicated. The import lzma trick won't work because if the lzma package is defined it doesn't imply the tarfile package uses it. The version check won't work because it doesn't imply that lzma support is compiled in. The least common denominator is gz-compressed tarballs.

Perhaps a configuration option could be used to opt-in to this? Or something like that?

[alex]

I think if you change return True after import lzma to return sys.version_info[0] >= 3 then this is correct under all practical circumstances.

[guanqun]

@alex we can do that, but I'd argue the logic is a bit unclear at first glance, just checking the python version is >= 3.3 is much clear.

@alexcrichton I don't see why the version check won't work, as lzma is part of the standard library of Python after that version.

[alexcrichton]

Presumably though lzma support can be disabled when python is compiled right? Sorry but this is where it all just seems like this is perhaps a bit too much effort with respect to the change proposed here?

[alex]

It's technically possible, yes. I'm practice compiling with lzma disabled is _incredibly_ rare.

…

On Mon, Nov 4, 2019, 9:55 AM Alex Crichton ***@***.***> wrote: Presumably though lzma support can be disabled when python is compiled right? Sorry but this is where it all just seems like this is perhaps a bit too much effort with respect to the change proposed here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65932?email_source=notifications&email_token=AAAAGBDUGPMBXGWH7AKX3STQSAZXXA5CNFSM4JGJNEC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC7QVBI#issuecomment-549390981>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBBWDKCFZMTCJZH7BDTQSAZXXANCNFSM4JGJNECQ> .

[JohnCSimon]

Ping from triage - this PR has sat idle for the last few days...
@alex @alexcrichton @guanqun
Can you post what else needs to happen with this PR?

Thanks!

[guanqun]

I come up with a slightly better idea. We can try to create a .tar.xz file, if it can succeed, then on the other hand, the decompression should be fine. With this approach, we can bypass the version check or the import check.

Code is pushed, but I need to test that with Python3, will report back.

[guanqun]

I tried python2 and python3, they both can figure out the right file types. These are pre-built version, not custom built, I believe it would cover most of the use cases.

[alexcrichton]

@bors: r+

[bors]

📌 Commit 8d56bcc has been approved by alexcrichton

[alexcrichton]

@bors: r-

er actually, let's keep this out of the hot path. This shouldn't be calculated every time ./x.py build is run.

[guanqun]

thank you for the review. I've moved the check to the downloading function. locally it's fine for python 3. let's see if the CI job works. it's using python 2.

[alexcrichton]

@bors: r+

[bors]

📌 Commit 0019371 has been approved by alexcrichton