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

refactor: add gen-source scripts/binary to avoid pinned deps #3926

Closed

Conversation

sunng87
Copy link

@sunng87 sunng87 commented Mar 24, 2023

Which issue does this PR close?

Closes #3876.

Rationale for this change

As mentioned in #3876 , the pinned version of build dependencies are causing version conflicts when there is a newer version in the same project.

What changes are included in this PR?

This patch makes our source generation independent from cargo build process. It follows a great practice from https://stackoverflow.com/a/53728256 . In this patch, source code generation are moved to a separated binary gen-source.

When we want to update the source code, simply run script ./gen-source.sh. Since our previous update process is also manual, the developer experience should be similar or even better .

Are there any user-facing changes?

No.

@github-actions github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Mar 24, 2023
@tustvold
Copy link
Contributor

tustvold commented Mar 24, 2023

Since our previous update process is also manual

Could we get a CI check to make sure things stay in sync, i.e. run gen sources and check git shows no changes? Something like this already exists, but assumes the build is running the generator

@@ -46,13 +46,19 @@ clap = { version = "4.1", default-features = false, features = ["std", "derive",
tracing-log = { version = "0.1", optional = true }
tracing-subscriber = { version = "0.3.1", default-features = false, features = ["ansi", "fmt"], optional = true }

# Build dependencies
proc-macro2 = { version = "1.0.53", default-features = false, optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we could split these into a separate child crate?

@@ -46,13 +46,19 @@ clap = { version = "4.1", default-features = false, features = ["std", "derive",
tracing-log = { version = "0.1", optional = true }
tracing-subscriber = { version = "0.3.1", default-features = false, features = ["ansi", "fmt"], optional = true }

# Build dependencies
proc-macro2 = { version = "1.0.53", default-features = false, optional = true }
prost-build = { version = "0.11.8", default-features = false, optional = true }
Copy link
Contributor

@tustvold tustvold Mar 24, 2023

Choose a reason for hiding this comment

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

These still need to be pinned, as otherwise the vendored code can drift. I think splitting them into a child crate is the way to go here

Copy link
Author

Choose a reason for hiding this comment

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

I think with the change we can use pinned version here with no harm. It won't affect runtime because it's gated by feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm currently bashing out the sub-crate version, I would rather avoid adding a public feature if possible

Copy link
Author

Choose a reason for hiding this comment

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

hmm.. makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

PR #3927 apologies for pre-empting you on this, trying to save some back and forth and really want this to make the release this afternoon 😄

Copy link
Author

Choose a reason for hiding this comment

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

Never mind! I hope this can be released as soon as possible too because we are blocked by this issue.

Happy Friday afternoon release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pinned version in arrow-flight's build-dependencies are causing conflicts
2 participants