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.

Sign up for GitHub

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

Jump to bottom

refactor: hard-code v1 API response #1914

Closed
wants to merge 1 commit into from
Closed

refactor: hard-code v1 API response #1914

wants to merge 1 commit into from

Conversation

weihanglo
Copy link
Member

Another step toward making rustc-perf self-contained.
See rust-lang/rust#125465.

It makes no different between depending on an arbitrary Git commit
of rust-lang/team and inlining the responded JSON structure.

@weihanglo
refactor: hard-code v1 API response
0df4e9a 
It makes no different between  depending on an arbitrary Git commit
of rust-lang/team and inlining the responded JSON structure.
@weihanglo
Copy link
Member Author

r? @Kobzol

@Kobzol
Copy link
Contributor

Kobzol commented May 29, 2024 •
edited
Loading

Hmm. I don't like this at all 😆 Especially since it's only for a git dependency that is being used by site, which isn't even required for rust-lang/rust.

Isn't it possible to vendor a git dependency in Cargo?

@weihanglo
Copy link
Member Author

weihanglo commented May 29, 2024 •
edited
Loading

Isn't it possible to vendor a git dependency in Cargo?

It is indeed supported. The caveats of doing that are:

  • It currently depends on both the team API and the Git repository, rather than just the team API. If the commit is gone (unlikely though), it then cannot be built. If there is a breaking change the team API, we're gonna change both. Depending on the Git repository doesn't really save us from breaking changes.
  • The Git source needs to be added to the exception source list in rust-lang/rust tidy check (See bootstrap: vendor crates required by opt-dist to collect profiles  rust#125465 (comment)).

it's only for a git dependency that is being used by site, which isn't even required for rust-lang/rust.

I am sorry this requires rust-lang/cargo#12307 😞.
I meant it is currently not possible to vendor only a subset of packages in a workspace.

I am fine with either way. We could alternatively add an exception in rust-lang/rust and work on rust-lang/cargo#12307. Feel free to merge or close :)

@Kobzol
Copy link
Contributor

Kobzol commented May 29, 2024

It currently depends on both the team API and the Git repository, rather than just the team API. If the commit is gone (unlikely though), it then cannot be built. If there is a breaking change the team API, we're gonna change both. Depending on the Git repository doesn't really save us from breaking changes.

To be clear, we don't really have issues with updating the team API in this repo, so I'm not too worried about the approach that we use with the git dependency. I'm just interested in getting vendoring working without having to hardcode stuff.

In rust-lang/rust, none of the team API code is used, therefore my idea was that if we just vendor the git reference at the time of packaging the source tarball, it should just work, right? We just need it to compile (in any way), it won't be actually used during benchmarking.

@weihanglo
Copy link
Member Author

True. Adding "git+https://github.com/rust-lang/team#89bb40d3dca1bd9a6c6dbc1b8cb2ddc1fcc932ad" to ALLOWED_SOURCES should be sufficient. Let's close then.

(I am wanting partial vendoring in Cargo more after this 😬)

@weihanglo weihanglo closed this May 29, 2024
@weihanglo weihanglo deleted the team-data branch May 29, 2024 14:08
@Kobzol
Copy link
Contributor

Kobzol commented May 29, 2024

Ok, thanks! That being said, not all git dependencies are really needed. It would be nice to resolve the messy measureme situation where we depend on three different versions.

@weihanglo weihanglo mentioned this pull request May 31, 2024
Merged
weihanglo added a commit to weihanglo/rust that referenced this pull request May 31, 2024
@weihanglo
tidy: add license exceptions for rustc-perf
1230123 
`ring` is included because it is an optional dependency of `hyer`,
which is a training data in rust-pref for optimized build.
The license of it is generally `ISC AND MIT AND OpenSSL`,
though the `package.license` field is not set.

See briansmith/ring#902

`git+https://github.com/rust-lang/team` git source is from
`rust_team_data`, which is used by `site` in src/tools/rustc-perf.
This doesn't really a part of the compiler,
so doesn't really affect the bootstrapping process.

See rust-lang/rustc-perf#1914.
@weihanglo weihanglo mentioned this pull request May 31, 2024
Merged
weihanglo added a commit to weihanglo/rust that referenced this pull request May 31, 2024
@weihanglo
tidy: add license exceptions for rustc-perf
e47af78 
`ring` is included because it is an optional dependency of `hyper`,
which is a training data in rustc-pref for optimized build.
The license of it is generally `ISC AND MIT AND OpenSSL`,
though the `package.license` field is not set.

See briansmith/ring#902

`git+https://github.com/rust-lang/team` git source is from
`rust_team_data`, which is used by `site` in src/tools/rustc-perf.
This doesn't really a part of the compiler,
so doesn't really affect the bootstrapping process.

See rust-lang/rustc-perf#1914.
weihanglo added a commit to weihanglo/rust that referenced this pull request Jun 5, 2024
@weihanglo
tidy: add license exceptions for rustc-perf
6799ee1 
`ring` is included because it is an optional dependency of `hyper`,
which is a training data in rustc-pref for optimized build.
The license of it is generally `ISC AND MIT AND OpenSSL`,
though the `package.license` field is not set.

See briansmith/ring#902

`git+https://github.com/rust-lang/team` git source is from
`rust_team_data`, which is used by `site` in src/tools/rustc-perf.
This doesn't really a part of the compiler,
so doesn't really affect the bootstrapping process.

See rust-lang/rustc-perf#1914.
weihanglo added a commit to weihanglo/rust that referenced this pull request Jun 9, 2024
@weihanglo
tidy: add license exceptions for rustc-perf
3778703 
`ring` is included because it is an optional dependency of `hyper`,
which is a training data in rustc-pref for optimized build.
The license of it is generally `ISC AND MIT AND OpenSSL`,
though the `package.license` field is not set.

See briansmith/ring#902

`git+https://github.com/rust-lang/team` git source is from
`rust_team_data`, which is used by `site` in src/tools/rustc-perf.
This doesn't really a part of the compiler,
so doesn't really affect the bootstrapping process.

See rust-lang/rustc-perf#1914.
lcnr pushed a commit to lcnr/rust that referenced this pull request Jun 12, 2024
@weihanglo @lcnr
tidy: add license exceptions for rustc-perf
4731c7e 
`ring` is included because it is an optional dependency of `hyper`,
which is a training data in rustc-pref for optimized build.
The license of it is generally `ISC AND MIT AND OpenSSL`,
though the `package.license` field is not set.

See briansmith/ring#902

`git+https://github.com/rust-lang/team` git source is from
`rust_team_data`, which is used by `site` in src/tools/rustc-perf.
This doesn't really a part of the compiler,
so doesn't really affect the bootstrapping process.

See rust-lang/rustc-perf#1914.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@weihanglo @Kobzol