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

Fix skipping over invalid registry packages #6912

Merged
merged 2 commits into from May 6, 2019

Conversation

Projects
None yet
4 participants
@alexcrichton
Copy link
Member

commented May 6, 2019

This accidentally regressed in the previous caching PR for Cargo.
Invalid lines of JSON in the registry are intended to be skipped over,
but when skipping we forgot to update some indices which meant that all
future versions would fail to parse as well!

@rust-highfive

This comment has been minimized.

Copy link

commented May 6, 2019

r? @Eh2406

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

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

Note that the first commit here has the actual fix and then the follow-up is a fix to make it more robust in general

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

The fix looks good, so lets merge soon.
This is the kind of thing I was worried about in the previous pr.
Is there any way to get some testing on this? How did you notice the problem?

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

It's true that this wasn't caught before! I honestly don't know how we'd catch it though. It'd pretty hard to catch bugs you don't know exist... We could always just simply add more tests but I have no idea how this would have been caught without an incredibly specific test for this exact scenario.

I basically just happened to catch this by a huge stroke of luck I think. I was curious if building wasm-bindgen with pipelining was faster (it wasn't) and it happened to include this change. Nothing was resolving because futures-util-preview didn't have the right version. Turns out some historic version contained a bug and failed to JSON parse, which previously just that line was ignored and this time it causes every line after it to be parsed incorrectly as well.

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

It'd pretty hard to catch bugs you don't know exist...

So true. So true.
Should we add a test that includes that file from the index and make sure we can use all the valid lines from it?

alexcrichton added some commits May 6, 2019

Fix skipping over invalid registry packages
This accidentally regressed in the previous caching PR for Cargo.
Invalid lines of JSON in the registry are intended to be skipped over,
but when skipping we forgot to update some indices which meant that all
future versions would fail to parse as well!
Make usage of `memchr` more robust
Use an iterator adaptor which is tasked with keeping track of indices
for us so we don't have to indice juggle elsewhere.

@alexcrichton alexcrichton force-pushed the alexcrichton:fix-parse branch from 17fc381 to 71c01fe May 6, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

Good point! I've added a test asserting such (failed before, passes after)

@Eh2406

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

📌 Commit 71c01fe has been approved by Eh2406

@bors

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

⌛️ Testing commit 71c01fe with merge 573f9bb...

bors added a commit that referenced this pull request May 6, 2019

Auto merge of #6912 - alexcrichton:fix-parse, r=Eh2406
Fix skipping over invalid registry packages

This accidentally regressed in the previous caching PR for Cargo.
Invalid lines of JSON in the registry are intended to be skipped over,
but when skipping we forgot to update some indices which meant that all
future versions would fail to parse as well!
@bors

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: Eh2406
Pushing 573f9bb to master...

@bors bors merged commit 71c01fe into rust-lang:master May 6, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@ehuss ehuss referenced this pull request May 7, 2019

Merged

Update cargo #60596

bors added a commit to rust-lang/rust that referenced this pull request May 7, 2019

Auto merge of #60596 - ehuss:update-cargo, r=alexcrichton
Update cargo

12 commits in beb8fcb5248dc2e6aa488af9613216d5ccb31c6a..759b6161a328db1d4863139e90875308ecd25a75
2019-04-30 23:58:00 +0000 to 2019-05-06 20:47:49 +0000
- Small things (rust-lang/cargo#6910)
- Fix skipping over invalid registry packages (rust-lang/cargo#6912)
- Fixes rust-lang/cargo#6874 (rust-lang/cargo#6905)
- doc: Format examples of version to ease reading (rust-lang/cargo#6907)
- fix more typos (codespell) (rust-lang/cargo#6903)
- Parse less JSON on null builds (rust-lang/cargo#6880)
- chore: Update opener to 0.4 (rust-lang/cargo#6902)
- Update documentation for auto-discovery. (rust-lang/cargo#6898)
- Update some doc links. (rust-lang/cargo#6897)
- Default Cargo.toml template provide help for completing the metadata (rust-lang/cargo#6881)
- Run 'cargo fmt --all' (rust-lang/cargo#6896)
- Refactor command definition (rust-lang/cargo#6894)

@alexcrichton alexcrichton deleted the alexcrichton:fix-parse branch Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.