Re: [PR] Minor: Document the rationale for the lack of Cargo.lock [datafusion]
alamb commented on code in PR #14071: URL: https://github.com/apache/datafusion/pull/14071#discussion_r1917019302 ## README.md: ## @@ -146,3 +146,27 @@ stable API, we also improve the API over time. As a result, we typically deprecate methods before removing them, according to the [deprecation guidelines]. [deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html + +## Dependencies and a `Cargo.lock` Review Comment: I filed a ticket to discuss next steps: - https://github.com/apache/datafusion/issues/14135 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Minor: Document the rationale for the lack of Cargo.lock [datafusion]
comphead merged PR #14071: URL: https://github.com/apache/datafusion/pull/14071 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Minor: Document the rationale for the lack of Cargo.lock [datafusion]
comphead commented on code in PR #14071: URL: https://github.com/apache/datafusion/pull/14071#discussion_r1915282401 ## README.md: ## @@ -146,3 +146,27 @@ stable API, we also improve the API over time. As a result, we typically deprecate methods before removing them, according to the [deprecation guidelines]. [deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html + +## Dependencies and a `Cargo.lock` + +`datafusion` is intended for use as a library and thus purposely does not have a +`Cargo.lock` file checked in. You can read more about the distinction in the +[Cargo book]. + +CI tests always run against the latest compatible versions of all dependencies +(the equivalent of doing `cargo update`), as suggested in the [Cargo CI guide] +and we rely on Dependabot for other upgrades. This strategy has two problems +that occasionally arise: + +2. CI failures when downstream libraries upgrade in some non compatible way +3. Local development builds that fail when DataFusion inadvertently relies on Review Comment: ```suggestion 1. CI failures when downstream libraries upgrade in some non compatible way 2. Local development builds that fail when DataFusion inadvertently relies on ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Minor: Document the rationale for the lack of Cargo.lock [datafusion]
alamb commented on code in PR #14071: URL: https://github.com/apache/datafusion/pull/14071#discussion_r1912129419 ## README.md: ## @@ -146,3 +146,27 @@ stable API, we also improve the API over time. As a result, we typically deprecate methods before removing them, according to the [deprecation guidelines]. [deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html + +## Dependencies and a `Cargo.lock` Review Comment: One thing we have to be aware of in DataFusion is that as part of the Apache security posture, only certain third party actions are allowed -- we would have to double check Rennovate I think the next step is probably to file an issue to explicitly discuss checking in a Cargo.lock file. I'll try and find time over the next few days ## README.md: ## @@ -146,3 +146,27 @@ stable API, we also improve the API over time. As a result, we typically deprecate methods before removing them, according to the [deprecation guidelines]. [deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html + +## Dependencies and a `Cargo.lock` Review Comment: One thing we have to be aware of in DataFusion is that as part of the Apache security posture, only certain third party actions are allowed -- we would have to double check Rennovate I think the next step is probably to file an issue to explicitly discuss checking in a Cargo.lock file. I'll try and find time over the next few days if no one beats me to it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Minor: Document the rationale for the lack of Cargo.lock [datafusion]
gatesn commented on code in PR #14071: URL: https://github.com/apache/datafusion/pull/14071#discussion_r1911310930 ## README.md: ## @@ -146,3 +146,27 @@ stable API, we also improve the API over time. As a result, we typically deprecate methods before removing them, according to the [deprecation guidelines]. [deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html + +## Dependencies and a `Cargo.lock` Review Comment: Just my two cents, but I have found [Renovate](https://www.mend.io/renovate/) to be much more configurable. Here's an example of a lock file maintenance PR: https://github.com/spiraldb/vortex/pull/1818 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Minor: Document the rationale for the lack of Cargo.lock [datafusion]
gatesn commented on code in PR #14071: URL: https://github.com/apache/datafusion/pull/14071#discussion_r1911310930 ## README.md: ## @@ -146,3 +146,27 @@ stable API, we also improve the API over time. As a result, we typically deprecate methods before removing them, according to the [deprecation guidelines]. [deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html + +## Dependencies and a `Cargo.lock` Review Comment: Just my two cents, but I have found Renovate to be much more configurable. Here's an example of a lock file maintenance PR: https://github.com/spiraldb/vortex/pull/1818 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Minor: Document the rationale for the lack of Cargo.lock [datafusion]
alamb commented on code in PR #14071: URL: https://github.com/apache/datafusion/pull/14071#discussion_r1910442292 ## README.md: ## @@ -146,3 +146,27 @@ stable API, we also improve the API over time. As a result, we typically deprecate methods before removing them, according to the [deprecation guidelines]. [deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html + +## Dependencies and a `Cargo.lock` Review Comment: TLDR it sounds like the rust team now suggests always committing `Cargo.lock` and letting dependabot handle updates. That seems like a good idea to me -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Minor: Document the rationale for the lack of Cargo.lock [datafusion]
mbrobbel commented on code in PR #14071: URL: https://github.com/apache/datafusion/pull/14071#discussion_r1910296234 ## README.md: ## @@ -146,3 +146,27 @@ stable API, we also improve the API over time. As a result, we typically deprecate methods before removing them, according to the [deprecation guidelines]. [deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html + +## Dependencies and a `Cargo.lock` Review Comment: I'd like to point to https://blog.rust-lang.org/2023/08/29/committing-lockfiles.html for considerations and suggestions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Minor: Document the rationale for the lack of Cargo.lock [datafusion]
alamb commented on code in PR #14071: URL: https://github.com/apache/datafusion/pull/14071#discussion_r1910289206 ## README.md: ## @@ -146,3 +146,27 @@ stable API, we also improve the API over time. As a result, we typically deprecate methods before removing them, according to the [deprecation guidelines]. [deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html + +## Dependencies and a `Cargo.lock` Review Comment: From my perspective it is very important that CI covers testing with the latest versions of all dependencies (as that is what many/most downstream crates will use as well) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Minor: Document the rationale for the lack of Cargo.lock [datafusion]
alamb commented on code in PR #14071: URL: https://github.com/apache/datafusion/pull/14071#discussion_r1910286465 ## README.md: ## @@ -146,3 +146,27 @@ stable API, we also improve the API over time. As a result, we typically deprecate methods before removing them, according to the [deprecation guidelines]. [deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html + +## Dependencies and a `Cargo.lock` Review Comment: @mbrobbel has a good point here https://github.com/apache/datafusion/pull/14069#issuecomment-2582533271 I think one concern originally was how we would keep a `Cargo.lock` file up to date with the latest version of the dependencies I can't remember if depndabot was available at that point -- maybe dependabot is good enough that we could check in Cargo.lock and have Dependabot update it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
