-
Notifications
You must be signed in to change notification settings - Fork 577
upgrade rust to 1.85.0 #11295
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
upgrade rust to 1.85.0 #11295
Conversation
f1ac25a
to
c7fb2c2
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.
Can we please skip the failing tests, so that the pipeline is green.
Let's add a link where we skip to the FreeBSD forum to clarify on why are we even skiping :)
SPECS/flux/flux.spec
Outdated
@@ -71,23 +71,11 @@ programs using Influx data language. | |||
|
|||
%prep | |||
%setup -q | |||
%patch 2 -p1 |
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.
Potential for optimization but fine with me
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.
Is there a good reason for continuing to use %setup and %patch here instead of changing to %autosetup?
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.
Updated it to %autosetup
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 notice that you switched back away from %autosetup
.
I don't mind this, because it keeps us closer to the OpenSUSE upstream, but I wanted to make sure that's intentional.
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.
lgtm
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.
LGTM :)
🥇
d372d75
to
46c915c
Compare
SPECS/rust/rust.spec
Outdated
|
||
Patch0: Remove_cannot_write_error_test.patch |
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.
Patch0, 1: Re-verify if these fail in the pipeline. Both seem to have a similar issue (expect permission issues are revoking permissions with chmod, but the expected-to-fail commands still work)
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.
If tests fail, let's make a judgement on who and decide whether we want to fix resp. need to make some changes, or whether we'd want to accept this
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.
Test passed when run as non-root user
SPECS/rust/rust.spec
Outdated
|
||
Patch0: Remove_cannot_write_error_test.patch | ||
Patch1: Remove_leave_log_after_failure_test.patch | ||
Patch2: Ignore_failing_ci_tests.patch |
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.
Let's specify which CI tests fail and re-assess
SPECS/rust/rust.spec
Outdated
Patch1: Remove_leave_log_after_failure_test.patch | ||
Patch2: Ignore_failing_ci_tests.patch | ||
Patch3: skip-failing-run-make-tests.patch | ||
Patch4: Ignore-test-for-aarch64.patch |
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.
error: linker aarch64-linux-gnu-gcc
not found. Let's assess whether this is expected to fail for Azure Linux or e.g. whether the linker binary would need to be renamed.
Side note: Let's add some documentation for all the patch files we add. This way it becomes clear why we did this
SPECS/rust/rust.spec
Outdated
Patch0: Remove_cannot_write_error_test.patch | ||
Patch1: Remove_leave_log_after_failure_test.patch | ||
Patch2: Ignore_failing_ci_tests.patch | ||
Patch3: skip-failing-run-make-tests.patch |
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.
Let's document which of these are failing (three tests are failing) and re-assess.
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.
run-make tests passed when run as non-root user
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.
In retrospect, we should get to the bottom of why these patches removing tests are required before proceeding.
648a89f
to
46c915c
Compare
ca87822
to
f17dae5
Compare
d486f8c
to
74eaf3f
Compare
SPECS/flux/flux.spec
Outdated
@@ -43,6 +43,7 @@ Patch1: disable-static-library.patch | |||
# Fixed upstream in 1.195.0, https://github.com/influxdata/flux/pull/5484. | |||
Patch2: fix-build-warnings.patch | |||
Patch3: fix-unsigned-char.patch | |||
Patch4: 0001-Fix-with-latest-rust.patch |
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.
nit: Please fix spacing.
SPECS/flux/flux.spec
Outdated
@@ -71,23 +71,11 @@ programs using Influx data language. | |||
|
|||
%prep | |||
%setup -q | |||
%patch 2 -p1 |
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 notice that you switched back away from %autosetup
.
I don't mind this, because it keeps us closer to the OpenSUSE upstream, but I wanted to make sure that's intentional.
SPECS/flux/flux.spec
Outdated
pushd libflux | ||
tar -xf %{SOURCE1} | ||
install -D %{SOURCE2} .cargo/config | ||
|
||
patch -p2 < %{PATCH1} |
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.
Looks like we're no longer applying PATCH1
.
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.
Removed it mistakenly, adding it again
SPECS/flux/flux.spec
Outdated
@@ -93,6 +94,7 @@ patch -p2 <<EOF | |||
} | |||
EOF | |||
|
|||
|
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.
nit: remove extra blank line.
05eb833
to
43ca753
Compare
43ca753
to
6277f76
Compare
3e83972
to
c1b4f6d
Compare
9fe9ef3
to
3a96e33
Compare
%define release_date 2023-11-16 | ||
%define stage0_version 1.74.0 | ||
%define release_date 2025-01-09 | ||
%define stage0_version 1.84.0 |
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.
Is the 1.74.0 version capable of building the 1.85 version?
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.
Is the 1.74.0 version capable of building the 1.85 version?
No, it should be build with 1.84
Co-authored-by: kavyasree <kkaitepalli@microsoft.com>
Merge Checklist
All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)
*-static
subpackages, etc.) have had theirRelease
tag incremented../cgmanifest.json
,./toolkit/scripts/toolchain/cgmanifest.json
,.github/workflows/cgmanifest.json
)./LICENSES-AND-NOTICES/SPECS/data/licenses.json
,./LICENSES-AND-NOTICES/SPECS/LICENSES-MAP.md
,./LICENSES-AND-NOTICES/SPECS/LICENSE-EXCEPTIONS.PHOTON
)*.signatures.json
filessudo make go-tidy-all
andsudo make go-test-coverage
passSummary
What does the PR accomplish, why was it needed?
Change Log
Does this affect the toolchain?
NO
Associated issues
Links to CVEs
Test Methodology