-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add VARIABLE_DEFINITION directive location #66
Conversation
Summary: graphql-rust/graphql-parser#66 Reviewed By: zertosh Differential Revision: D37455822 fbshipit-source-id: f69d459c11d3e44166fbd72e70294adfc39ca4aa
Summary: graphql-rust/graphql-parser#66 Reviewed By: zertosh Differential Revision: D37455822 fbshipit-source-id: f69d459c11d3e44166fbd72e70294adfc39ca4aa
Summary: graphql-rust/graphql-parser#66 Reviewed By: zertosh Differential Revision: D37455822 fbshipit-source-id: f69d459c11d3e44166fbd72e70294adfc39ca4aa
Summary: My previous attempt D37455822 (21c109c) apparently broke reindeer, trying again graphql-rust/graphql-parser#66 Reviewed By: zertosh Differential Revision: D37532244 fbshipit-source-id: 755f48e91ce9a72acd2a782a12ee1f1e6ede254b
Summary: My previous attempt D37455822 (6a141e0) apparently broke reindeer, trying again graphql-rust/graphql-parser#66 Reviewed By: zertosh Differential Revision: D37532244 fbshipit-source-id: 755f48e91ce9a72acd2a782a12ee1f1e6ede254b
Summary: My previous attempt D37455822 (5526e05) apparently broke reindeer, trying again graphql-rust/graphql-parser#66 Reviewed By: zertosh Differential Revision: D37532244 fbshipit-source-id: 755f48e91ce9a72acd2a782a12ee1f1e6ede254b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Can we get a test to make sure this doesn't regress?
Perhaps this and #61 can be merged |
I have some schemas that have this directive location, and this crate currently crashes while trying to parse it.
@LegNeato I added a test that is similar to other directive parsers, let me know if you had something else in mind |
@LegNeato once this lands, any chance we could get a new version of this crate pushed to crates.io along with a new version of |
Summary: Currently, `Cargo.toml` files generated by `autocargo` contain the following line: https://www.internalfb.com/code/fbsource/[159701a8d991]/fbcode/eden/scm/public_autocargo/Cargo.toml?lines=7 At the time of this writing, GitHub still appears to know about `1d155d96e6052767380ab5e67c57e3d6608a31ac`, as it was recently "orphaned" (i.e., it does not belong to a branch), but because it is "orphaned," it appears that GitHub refuses to tell `cargo` about it when it requests it, which breaks builds. To create this diff, I updated the `[patch.crates-io]` section in `third-party/rust/Cargo.toml` and then ran `fbcode/common/rust/cargo_from_buck/bin/autocargo`. I replaced `1d155d96e6052767380ab5e67c57e3d6608a31ac` with the new head of graphql-rust/graphql-parser#66, which is `c778917f57f6b2c26d9291819b9bb341a2f5948a`. What happened? Here's what it looks like: - After discovering that our `Cargo.toml` files created by `autocargo` contained massive `[patch.crates-io]` sections that appeared to be slowing down our open source build, I went and posted about it in the Source Control Team group: https://fb.workplace.com/groups/sourcecontrolteam/posts/5310340079087295 - One way to improve the situation is to eliminate these patches from `//third-party/rust`, so I spot-checked a number of the patches, tracking down the diff that introduced them, and attempted to follow-up with the author to upstream the patch. - D37532244 (c7c08d9) appeared to be the diff that pulled in the patch for `graphql-parser`. In the comments, I pinged the author (vmagro) to see if he could upstream his patch. - Vinnie agreed and updated his PR: graphql-rust/graphql-parser#66 - *what appears to have happened* is that Vinnie updated the PR by doing a "force push" where the previous head was `1d155d96e6052767380ab5e67c57e3d6608a31ac` and the new head is `c778917f57f6b2c26d9291819b9bb341a2f5948a`. - GitHub...does not like force pushes. Indeed, if you visit graphql-rust/graphql-parser@1d155d9 you see a yellow warning banner at the top that says **This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.** I don't know what call `cargo` makes to resolve these `git` dependencies, but I assume GitHub is refusing to respond as it normally would for orphaned commits such as these. Reviewed By: fanzeyi Differential Revision: D39190591 fbshipit-source-id: b8d1187cdec9312e9215b28020a735058faeb2d7
Summary: Currently, `Cargo.toml` files generated by `autocargo` contain the following line: https://www.internalfb.com/code/fbsource/[159701a8d991]/fbcode/eden/scm/public_autocargo/Cargo.toml?lines=7 At the time of this writing, GitHub still appears to know about `1d155d96e6052767380ab5e67c57e3d6608a31ac`, as it was recently "orphaned" (i.e., it does not belong to a branch), but because it is "orphaned," it appears that GitHub refuses to tell `cargo` about it when it requests it, which breaks builds. To create this diff, I updated the `[patch.crates-io]` section in `third-party/rust/Cargo.toml` and then ran `fbcode/common/rust/cargo_from_buck/bin/autocargo`. I replaced `1d155d96e6052767380ab5e67c57e3d6608a31ac` with the new head of graphql-rust/graphql-parser#66, which is `c778917f57f6b2c26d9291819b9bb341a2f5948a`. What happened? Here's what it looks like: - After discovering that our `Cargo.toml` files created by `autocargo` contained massive `[patch.crates-io]` sections that appeared to be slowing down our open source build, I went and posted about it in the Source Control Team group: https://fb.workplace.com/groups/sourcecontrolteam/posts/5310340079087295 - One way to improve the situation is to eliminate these patches from `//third-party/rust`, so I spot-checked a number of the patches, tracking down the diff that introduced them, and attempted to follow-up with the author to upstream the patch. - D37532244 (8a740fa) appeared to be the diff that pulled in the patch for `graphql-parser`. In the comments, I pinged the author (vmagro) to see if he could upstream his patch. - Vinnie agreed and updated his PR: graphql-rust/graphql-parser#66 - *what appears to have happened* is that Vinnie updated the PR by doing a "force push" where the previous head was `1d155d96e6052767380ab5e67c57e3d6608a31ac` and the new head is `c778917f57f6b2c26d9291819b9bb341a2f5948a`. - GitHub...does not like force pushes. Indeed, if you visit graphql-rust/graphql-parser@1d155d9 you see a yellow warning banner at the top that says **This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.** I don't know what call `cargo` makes to resolve these `git` dependencies, but I assume GitHub is refusing to respond as it normally would for orphaned commits such as these. Reviewed By: fanzeyi Differential Revision: D39190591 fbshipit-source-id: b8d1187cdec9312e9215b28020a735058faeb2d7
Summary: Currently, `Cargo.toml` files generated by `autocargo` contain the following line: https://www.internalfb.com/code/fbsource/[159701a8d991]/fbcode/eden/scm/public_autocargo/Cargo.toml?lines=7 At the time of this writing, GitHub still appears to know about `1d155d96e6052767380ab5e67c57e3d6608a31ac`, as it was recently "orphaned" (i.e., it does not belong to a branch), but because it is "orphaned," it appears that GitHub refuses to tell `cargo` about it when it requests it, which breaks builds. To create this diff, I updated the `[patch.crates-io]` section in `third-party/rust/Cargo.toml` and then ran `fbcode/common/rust/cargo_from_buck/bin/autocargo`. I replaced `1d155d96e6052767380ab5e67c57e3d6608a31ac` with the new head of graphql-rust/graphql-parser#66, which is `c778917f57f6b2c26d9291819b9bb341a2f5948a`. What happened? Here's what it looks like: - After discovering that our `Cargo.toml` files created by `autocargo` contained massive `[patch.crates-io]` sections that appeared to be slowing down our open source build, I went and posted about it in the Source Control Team group: https://fb.workplace.com/groups/sourcecontrolteam/posts/5310340079087295 - One way to improve the situation is to eliminate these patches from `//third-party/rust`, so I spot-checked a number of the patches, tracking down the diff that introduced them, and attempted to follow-up with the author to upstream the patch. - D37532244 (199330f) appeared to be the diff that pulled in the patch for `graphql-parser`. In the comments, I pinged the author (vmagro) to see if he could upstream his patch. - Vinnie agreed and updated his PR: graphql-rust/graphql-parser#66 - *what appears to have happened* is that Vinnie updated the PR by doing a "force push" where the previous head was `1d155d96e6052767380ab5e67c57e3d6608a31ac` and the new head is `c778917f57f6b2c26d9291819b9bb341a2f5948a`. - GitHub...does not like force pushes. Indeed, if you visit graphql-rust/graphql-parser@1d155d9 you see a yellow warning banner at the top that says **This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.** I don't know what call `cargo` makes to resolve these `git` dependencies, but I assume GitHub is refusing to respond as it normally would for orphaned commits such as these. Reviewed By: fanzeyi Differential Revision: D39190591 fbshipit-source-id: b8d1187cdec9312e9215b28020a735058faeb2d7
Summary: Currently, `Cargo.toml` files generated by `autocargo` contain the following line: https://www.internalfb.com/code/fbsource/[159701a8d991]/fbcode/eden/scm/public_autocargo/Cargo.toml?lines=7 At the time of this writing, GitHub still appears to know about `1d155d96e6052767380ab5e67c57e3d6608a31ac`, as it was recently "orphaned" (i.e., it does not belong to a branch), but because it is "orphaned," it appears that GitHub refuses to tell `cargo` about it when it requests it, which breaks builds. To create this diff, I updated the `[patch.crates-io]` section in `third-party/rust/Cargo.toml` and then ran `fbcode/common/rust/cargo_from_buck/bin/autocargo`. I replaced `1d155d96e6052767380ab5e67c57e3d6608a31ac` with the new head of graphql-rust/graphql-parser#66, which is `c778917f57f6b2c26d9291819b9bb341a2f5948a`. What happened? Here's what it looks like: - After discovering that our `Cargo.toml` files created by `autocargo` contained massive `[patch.crates-io]` sections that appeared to be slowing down our open source build, I went and posted about it in the Source Control Team group: https://fb.workplace.com/groups/sourcecontrolteam/posts/5310340079087295 - One way to improve the situation is to eliminate these patches from `//third-party/rust`, so I spot-checked a number of the patches, tracking down the diff that introduced them, and attempted to follow-up with the author to upstream the patch. - D37532244 appeared to be the diff that pulled in the patch for `graphql-parser`. In the comments, I pinged the author (vmagro) to see if he could upstream his patch. - Vinnie agreed and updated his PR: graphql-rust/graphql-parser#66 - *what appears to have happened* is that Vinnie updated the PR by doing a "force push" where the previous head was `1d155d96e6052767380ab5e67c57e3d6608a31ac` and the new head is `c778917f57f6b2c26d9291819b9bb341a2f5948a`. - GitHub...does not like force pushes. Indeed, if you visit graphql-rust/graphql-parser@1d155d9 you see a yellow warning banner at the top that says **This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.** I don't know what call `cargo` makes to resolve these `git` dependencies, but I assume GitHub is refusing to respond as it normally would for orphaned commits such as these. Reviewed By: fanzeyi Differential Revision: D39190591 fbshipit-source-id: b8d1187cdec9312e9215b28020a735058faeb2d7
Summary: Currently, the open source build contains this line in a `Cargo.toml` file: https://github.com/facebook/sapling-staging/blob/c403b32adb27eb0c2d8a6a63261b9bf8942138ca/eden/scm/Cargo.toml#L7 For posterity, the contents of the line are: ``` graphql-parser = { git = "https://github.com/vmagro/graphql-parser", rev = "1d155d96e6052767380ab5e67c57e3d6608a31ac" } ``` The `cargo fetch` calls I am removing in this diff *used to* work. But today when I ran the workflow that used this `Dockerfile`, they failed with: ``` #13 4.589 error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index` #13 4.589 #13 4.589 Caused by: #13 4.589 failed to load source for dependency `graphql-parser` #13 4.589 #13 4.589 Caused by: #13 4.589 Unable to update https://github.com/vmagro/graphql-parser?rev=1d155d96e6052767380ab5e67c57e3d6608a31ac #13 4.589 #13 4.589 Caused by: #13 4.589 revspec '1d155d96e6052767380ab5e67c57e3d6608a31ac' not found; class=Reference (4); code=NotFound (-3) #13 ERROR: process "/bin/sh -c cargo fetch --manifest-path eden/scm/exec/hgmain/Cargo.toml" did not complete successfully: exit code: 101 ------ > [ 8/17] RUN cargo fetch --manifest-path eden/scm/exec/hgmain/Cargo.toml: #13 4.589 error: failed to resolve patches for `https://github.com/rust-lang/crates.io-index` #13 4.589 #13 4.589 Caused by: #13 4.589 failed to load source for dependency `graphql-parser` #13 4.589 #13 4.589 Caused by: #13 4.589 Unable to update https://github.com/vmagro/graphql-parser?rev=1d155d96e6052767380ab5e67c57e3d6608a31ac #13 4.589 #13 4.589 Caused by: #13 4.589 revspec '1d155d96e6052767380ab5e67c57e3d6608a31ac' not found; class=Reference (4); code=NotFound (-3) ``` What happened? Here's what it looks like: - After discovering that our `Cargo.toml` files created by `autocargo` contained massive `[patch.crates-io]` sections that appeared to be slowing down our open source build, I went and posted about it in the Source Control Team group: https://fb.workplace.com/groups/sourcecontrolteam/posts/5310340079087295 - One way to improve the situation is to eliminate these patches from `//third-party/rust`, so I spot-checked a number of the patches, tracking down the diff that introduced them, and attempted to follow-up with the author to upstream the patch. - D37532244 (199330f) appeared to be the diff that pulled in the patch for `graphql-parser`. In the comments, I pinged the author (vmagro) to see if he could upstream his patch. - Vinnie agreed and updated his PR: graphql-rust/graphql-parser#66 - *what appears to have happened* is that Vinnie updated the PR by doing a "force push" where the previous head was `1d155d96e6052767380ab5e67c57e3d6608a31ac` and the new head is `c778917f57f6b2c26d9291819b9bb341a2f5948a`. - GitHub...does not like force pushes. Indeed, if you visit graphql-rust/graphql-parser@1d155d9 you see a yellow warning banner at the top that says **This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.** I don't know what call `cargo` makes to resolve these `git` dependencies, but I assume GitHub is refusing to respond as it normally would for orphaned commits such as these. D39190591 (f500204) is the for fbsource overall, but based on this experience, I still think it is best to remove these `cargo fetch` calls. Of note, **even though the `cargo fetch` calls failed, the GitHub action to build Sapling still succeeds** because Sapling does not depend on `graphql-parser`, so `cargo build` only builds what it needs whereas `cargo fetch` fetches everything in the `Cargo.toml` whether it needs it or not. In sum, given the current state of `autocargo` and the `[patch.crates-io]` section it generates, `cargo fetch` just seems like a giant liability, so we are better off without it. Reviewed By: fanzeyi Differential Revision: D39189595 fbshipit-source-id: cbdf3c51e39b7ed2e49a4373e7f9fc66337766cc
Summary: vmagro deleted his forked and now broke Reindeer importing. Please be more careful. graphql-rust/graphql-parser#66 has been merged but not released. Reviewed By: dtolnay Differential Revision: D44198593 fbshipit-source-id: 287f0f3f6860cdf16deb7be80448c2962a9f10a6
Summary: vmagro deleted his forked and now broke Reindeer importing. Please be more careful. graphql-rust/graphql-parser#66 has been merged but not released. Reviewed By: dtolnay Differential Revision: D44198593 fbshipit-source-id: 287f0f3f6860cdf16deb7be80448c2962a9f10a6
Summary: vmagro deleted his forked and now broke Reindeer importing. Please be more careful. graphql-rust/graphql-parser#66 has been merged but not released. Reviewed By: dtolnay Differential Revision: D44198593 fbshipit-source-id: 287f0f3f6860cdf16deb7be80448c2962a9f10a6
@LegNeato I think this PR was never included in a release? any chance we can get a release for that fix? |
I have some schemas that have this directive location, and this crate
currently crashes while trying to parse it.