-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
chore(dev): remove 'built' dep and stop needlessly rebuilding on unrelated changes #7602
Conversation
…ted changes Signed-off-by: Toby Lawrence <toby@nuclearfurnace.com>
…dd everything back by hand Signed-off-by: Toby Lawrence <toby@nuclearfurnace.com>
34573e8
to
ee69024
Compare
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.
I like this. I'm not aware that we've made any guarantees about the vector build string so I tagged the community team on this PR, but I'm pleased with this work.
Nice! I'll give this a closer look tomorrow, but we should make sure the nightly builds still have the git sha in them as that's the only way to differentiate them at the moment. It's nice to have the build date in them too for quick reference. |
I can tell you off the bat I didn't add that back, but I can fix that quickly. :) |
Signed-off-by: Toby Lawrence <toby@nuclearfurnace.com>
Signed-off-by: Toby Lawrence <toby@nuclearfurnace.com>
Signed-off-by: Toby Lawrence <toby@nuclearfurnace.com>
Signed-off-by: Toby Lawrence <toby@nuclearfurnace.com>
Signed-off-by: Toby Lawrence <toby@nuclearfurnace.com>
Signed-off-by: Toby Lawrence <toby@nuclearfurnace.com>
Signed-off-by: Toby Lawrence <toby@nuclearfurnace.com>
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.
Awesome, thanks! This looks good to me. My only additional suggestion would be to maybe ensure the expected build info is set in the verify steps of the nightly/release workflows:
Not a big deal though since we weren't currently verifying that anyway with built
.
That's a good idea. 👍🏻 I can add that as an immediate follow-up item because I need to merge this to actually test the nightly flow. |
@tobz sounds good. For reference, you can actually trigger the nightly flow manually; which it seems like you already discovered 😄 |
As discovered in #7582, we often needlessly rebuild benchmark binaries in CI runs due to unrelated changes in code. This could also happen locally if any code within the "package" changed -- which, for most things, is anything in the repository at all -- as we have a build script, and build script default behavior involves rerunning itself when any file in the package changes. As the build script would run, values in the output would often change, leading then to a legitimate change that necessitated a rebuild. Ergo, if we can pare down what the build script spits out, and inform Cargo when it should rerun the build script, we can often avoid these binary rebuilds.
With that said, this PR introduces changes that remove
built
and add in our own, pared-down code for collecting and emitting build-time constants. While most of the information can be pulled natively from Cargo-provided environment variables, there is information we've given up, namely Git information and build time.As a concession, we've introduced a new environment variable --
VECTOR_BUILD_DESC
-- which can be overridden at any time, and allows any custom information to be provided, which ultimately is included as a sort of build ID in version strings. This could be set when building debug-friendly binaries if needed for support purposes, or overridden during versioned release builds to include compiler information, build time, and more.