Improve error messaging on bad Pants version

[navijation]

Resolves #155

Handle the case where the Github API returns multiple results due to an inexact release tag match
by raising a new TagNotFoundError which also links the Pants releases page. Also wrap the typical
CalledProcessError with this new exception.

[jsirois]

Thanks @navneethjayendran. Are you up for adding a test?

That might go here as a call to a new test_bad_pants_version test function you'd add lower down:

scie-pants/package/src/test.rs

Line 98 in b92a076

test_pants_sha(scie_pants_scie);

[jsirois]

It seems like it would make a bit more sense to keep the comment mostly unchanged but change the test to if not isinstance(github_api_response, dict) since that is what is used / relied upon on line 138 below. As it stands, if the API returned a string a bool or a number, the same style of confusing exception as this fixes would be raised.

[simon-liebehenschel]

Thank you for this pull request. It will definitely improve a user experience.

It will be also great to improve the documentation page https://www.pantsbuild.org/docs/initial-configuration#create-pantstoml because "$PANTS_VERSION" is confusing. I set the value to 2.15 expecting that I can set the version that I see on the documentation page, but, as it turns out, this must be the exact Git tag (2.15.1). Adding a URL to the "releases" Github page may be enough to clarify the case.

[navijation]

@jsirois Sorry for the long response time. I adjusted the check to look for a dict specifically. @AIGeneratedUsername I added a test case for this error message as you suggested.

[jsirois]

@navneethjayendran thanks for picking this back up. It looks like there are test issue to work out still. I'm going to add a few folks as reviewers since I've stepped away from Pants-related development. They should be able to help you take this home or else recommend further reviewers.

[kaos]

I worry that this does no longer make the same assertion about the sub process' exit code as before for all the unrelated tests using this method now.

[navijation]

Would adding a separate assertion to those call sites make sense? It seems natural for a macro that inspects stderr contents to tolerate nonzero exit codes.

[huonw]

As of #296, callers of assert_stderr_output can indicate whether the command should either succeed or fail (and both are checked).

[thejcannon]

So we're actually about to switch to a GitHub Release-based approach for any version >=2.

If you're able to sit a smidge, this code might look different when we make that switch.

[navijation]

Let me know when that happens and I can revisit.

[huonw]

Thanks for waiting! The switch has happened now.

For version strings that look new (after PANTS_PEX_GITHUB_RELEASE_VERSION), this code path isn't used, as the function returns early. For older ones, definitely still worth considering improvements'; thanks!

For those new ones, Pants is then downloaded from GitHub directly, with some error handling about it not existing (but feel free to fine-tune if you have ideas!), in

scie-pants/tools/src/scie_pants/install_pants.py

Lines 88 to 116 in 91477f6

	pex_url = f"https://github.com/pantsbuild/pants/releases/download/release_{version}/{pex_name}"
	if bootstrap_urls_path:
	bootstrap_urls = json.loads(Path(bootstrap_urls_path).read_text())
	urls_info = bootstrap_urls["ptex"]
	pex_url = urls_info.get(pex_name)
	if pex_url is None:
	raise ValueError(
	f"Couldn't find '{pex_name}' in PANTS_BOOTSTRAP_URLS file: '{bootstrap_urls_path}' "
	"under the 'ptex' key."
	)
	if not isinstance(pex_url, str):
	raise TypeError(
	f"The value for the key '{pex_name}' in PANTS_BOOTSTRAP_URLS file: '{bootstrap_urls_path}' "
	f"under the 'ptex' key was expected to be a string. Got a {type(pex_url).__name__}"
	)
	with tempfile.NamedTemporaryFile(suffix=".pex") as pants_pex:
	try:
	ptex.fetch_to_fp(pex_url, pants_pex.file)
	except subprocess.CalledProcessError as e:
	fatal(
	f"Wasn't able to fetch the Pants PEX at {pex_url}.\n\n"
	"Check to see if the URL is reachable (i.e. GitHub isn't down) and if"
	f" {pex_name} asset exists within the release."
	" If the asset doesn't exist it may be that this platform isn't yet supported."
	" If that's the case, please reach out on Slack: https://www.pantsbuild.org/docs/getting-help#slack"
	" or file an issue on GitHub: https://github.com/pantsbuild/pants/issues/new/choose.\n\n"
	f"Exception:\n\n{e}"
	)

[benjyw]

Hi @navneethjayendran ! Just checking on the status of this?