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.

Sign up for GitHub

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

Jump to bottom

ci: bump docker and docker-dind to 20.10.20 for gitlab #5045

Merged
merged 1 commit into from
Nov 2, 2022
Merged

ci: bump docker and docker-dind to 20.10.20 for gitlab #5045

merged 1 commit into from
Nov 2, 2022

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Oct 16, 2022 •
edited
Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@PastaPastaPasta
Copy link
Member

why?

@kwvg
Copy link
Collaborator Author

kwvg commented Oct 16, 2022 •
edited
Loading

Required for #5044, see ac1f853's comment

Primarily motivated by documented libstdc++ bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90415) and referenced https://stackoverflow.com/q/60862278 and https://blog.vrqq.org/archives/804/

Current version of Docker (19.03.5) won't work as Jammy compatibility was added in 20.10.13 (changelog) and alongside "improperly passed syscalls" (at least, based on this answer).

Currently, with the bug, builds are failing (for example, this GitLab CI job) and the solution (per my testing) was to update to GCC 10 and while I can do that for all cross compilation targets, I cannot do that for Windows cross compilation (see package), the same way I can for ARM64 cross compilation (see package)

@PastaPastaPasta
Copy link
Member

I don't think this makes senes. Looks like 9.5.0 of gcc has the bug fix also. I really don't think we should upgrade to ubuntu 22.04 at this point.

@kwvg
Copy link
Collaborator Author

kwvg commented Oct 16, 2022 •
edited
Loading

I am currently not aware of any way to force "non-native" targets (read, mingw-w64) to deviate from the base distribution's standard library, the "base" for focal is 9.4.0-1ubuntu1~20.04.1 as of now, which is still affected by the bug. To the best of my knowledge, the least inelegant way of upgrading the base libraries is by upgrading the distro entirely.

@PastaPastaPasta
Copy link
Member

What commit introduces the buggy code?

@kwvg
Copy link
Collaborator Author

kwvg commented Oct 16, 2022

It's not our code that's buggy, rather it's our code that trips the bug. But regardless, that would be 711f929, specifically portions unchanged by the commit that are affected by it, that are Dash-specific, like

dash/src/rpc/rpcevo.cpp

Line 1219 in 711f929

const JSONRPCRequest new_request{request.strMethod == "protx" ? request.squashed() : request};

With Ubuntu 22.04, these types of constructions build just fine with tests passing (tested locally within WSL2)

@PastaPastaPasta
Copy link
Member

Are there alternative fixes? maybe by changing how we initialize these requests? How did bitcoin-core avoid this buggy compiler code?

@kwvg
Copy link
Collaborator Author

kwvg commented Oct 16, 2022

Yes, but that workaround (using the assignment operator) doesn't work for us for some reason, it gives a different error instead.

@kwvg
Copy link
Collaborator Author

kwvg commented Oct 30, 2022

The bug in question has been sidestepped with #5055, this PR has been modified to only bump the underlying version of Docker, which adds Jammy support but our containers will still remain on Focal.

@kwvg kwvg changed the title contrib: bump docker base to latest ubuntu LTS (22.04), use gcc10 ci: bump docker and docker-dind to 20.10.20 for gitlab Oct 30, 2022
@kwvg kwvg requested a review from strophy October 30, 2022 16:34
strophy
strophy previously approved these changes Oct 30, 2022
View reviewed changes
Copy link

@strophy strophy left a comment

Choose a reason for hiding this comment

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

utACK, let's try it and see if it works. Manager node might need upgrading.

@UdjinM6 UdjinM6 mentioned this pull request Oct 30, 2022
Merged
5 tasks
UdjinM6
UdjinM6 requested changes Oct 31, 2022
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

blocking this with a "request changes" review to make sure it's not merged prior to #5068 (or any alternative PR) which fixes CI failures introduced by these changes

@github-actions
Copy link

This pull request has conflicts, please rebase.

@kwvg kwvg requested review from UdjinM6 and strophy November 1, 2022 05:21
@kwvg kwvg marked this pull request as ready for review November 1, 2022 05:22
strophy
strophy previously approved these changes Nov 1, 2022
View reviewed changes
Copy link

@strophy strophy left a comment

Choose a reason for hiding this comment

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

👍

@kwvg kwvg requested a review from PastaPastaPasta November 1, 2022 09:32
@PastaPastaPasta
Copy link
Member

CI is currently failing... Rebasing to see if that helps for some reason https://gitlab.com/dashpay/dash/-/jobs/3256133934

UdjinM6
UdjinM6 approved these changes Nov 1, 2022
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 added this to the 18.2 milestone Nov 1, 2022
PastaPastaPasta
PastaPastaPasta approved these changes Nov 2, 2022
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK for squash merge

@PastaPastaPasta PastaPastaPasta merged commit e74c41b into dashpay:develop Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@strophy strophy strophy left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
18.2
Development

Successfully merging this pull request may close these issues.
4 participants
@kwvg @PastaPastaPasta @UdjinM6 @strophy