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

Include rustc version in the user agent, if rustc is available #9987

Merged
merged 4 commits into from
May 21, 2021

Conversation

alex
Copy link
Member

@alex alex commented May 17, 2021

Rust is becoming more popular for writing Python extension modules in, this information would be valuable for package maintainers to assess the ecosystem, in the same way glibc or openssl version is.

Does this requires a newsfragment for pip? I'm not sure whether this is considered a user-facing change or not.

Rust is becoming more popular for writing Python extension modules in, this information would be valuable for package maintainers to assess the ecosystem, in the same way glibc or openssl version is.
Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

I'm neutral on whether we should include this, but in general it looks fine. Should we not add a test to verify that the information is being included correctly? I don't know if we have other user-agent checks, TBH.

@@ -598,13 +598,14 @@ def make_var(name):
with open(tmpdir.joinpath('req1.txt'), 'w') as fp:
fp.write(template.format(*map(make_var, env_vars)))

session = PipSession()
Copy link
Member

Choose a reason for hiding this comment

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

Changes in this file around PipSession seem unrelated?

Copy link
Member Author

Choose a reason for hiding this comment

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

They're not -- PipSession now checks for an env var (specifically PATHEXT on Windows), and these were being constructed inside a monkey-patch of getenv (which was intended to test parse_requirements).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. Can you add a comment explaining this, as it seems like a very subtle interaction that would be incredibly hard to diagnose if someone later decided to omit the extra variable as part of a tidy-up.

Actually, does that mean we're doing the subprocess call every time we construct a PipSession? Would it not be better to just calculate the rust version once, as it's not as if it's going to change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

And yes, we do it once per PipSession. While we construct a few of them in tests, I think in pip itself, it's only ever constructed once, so I'm not sure it's a big deal?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, probably not. Test suite times are dominated by running pip over and over, so a couple of extra rustc runs isn't going to harm anything.

@uranusjr
Copy link
Member

Does this requires a newsfragment for pip? I'm not sure whether this is considered a user-facing change or not.

I feel we should. Some people care very much about what local information is sent (for telemetry stuff) so we should be as transparent about this as possible.

@uranusjr
Copy link
Member

How do you intend to retrieve the message, via PyPI’s BigQuery? I’m wondering whether we should mention this in the fragment.

Implementation-wise LGTM.

@alex
Copy link
Member Author

alex commented May 17, 2021

Yeah, bigquery. I looked at the full changelog, and bigquery isn't mentioned for the other UA changes.

@dstufft dstufft merged commit 31482d5 into pypa:main May 21, 2021
@alex alex deleted the patch-1 branch May 21, 2021 03:22
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2021
@di
Copy link
Member

di commented Mar 23, 2022

For anyone interested, this is now live in PyPI's public dataset for both downloads and simple requests:

SELECT
  details.rustc_version,
  COUNT(*) AS total_downloads
FROM
  `bigquery-public-data.pypi.file_downloads`
WHERE
  DATE(timestamp) BETWEEN DATE_SUB(CURRENT_DATE(), INTERVAL 1 DAY)
  AND CURRENT_DATE()
GROUP BY
  details.rustc_version
ORDER BY
  total_downloads DESC
Row rustc_version total_downloads
1 null 904207816
2 1.59.0 992600
3 1.58.0 31051
4 1.56.1 15052
5 1.58.1 13760
6 1.47.0 13700
7 1.57.0 8510
8 1.56.1 4600
9 1.52.1 4078
10 1.35.0 3589

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants