Skip to content

Conversation

@roopakv
Copy link
Contributor

@roopakv roopakv commented Aug 19, 2021

What does this PR do?

Updates x/sys to support go 1.17 and unblock brew. More details here: Homebrew/homebrew-core#83413

Motivation

brew is blocked

Additional Notes

Please cut a release after merging this

@ldez ldez changed the title Replace x/sys for go 1.17 Update x/sys to support go 1.17 Aug 19, 2021
@ldez ldez changed the base branch from master to v2.5 August 19, 2021 07:45
@ldez ldez added this to the 2.5 milestone Aug 19, 2021
@mpl
Copy link
Collaborator

mpl commented Aug 19, 2021

@roopakv
Hello.
Looking at Homebrew/homebrew-core#83413 , I'm a bit confused by your diagnostic, especially since this PR is lacking details about what is failing. Notably:

  1. it seems like when you were discussing this issue, and if I'm looking at the history on https://github.com/Homebrew/homebrew-core/commits/c1700179ceb4878d580dac84fa996fd41e898914/Formula/traefik.rb , when you were running these tests, you were not on traefik 2.5, but on traefik 2.4 (which does not support Go 1.17). Am I wrong, can you clarify that?

  2. As of now, if I'm running brew test (on Catalina) with:

mbp:traefik mpl$ brew info traefik
traefik: stable 2.5.0 (bottled), HEAD
Modern reverse proxy
https://traefik.io/
/usr/local/Cellar/traefik/2.5.0 (8 files, 99.4MB) *
  Poured from bottle on 2021-08-19 at 15:00:43
From: https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/traefik.rb

I get

mbp:traefik mpl$ brew test --retry --verbose traefik
/usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.6.3_2/bin/bundle install
Fetching gem metadata from https://rubygems.org/.........
...
Installing rubocop-performance 1.11.5
Bundle complete! 33 Gemfile dependencies, 72 gems now installed.
Bundled gems are installed into `../../../../../../usr/local/Homebrew/Library/Homebrew/vendor/bundle`
Removing rubocop-performance (1.11.4)
==> Testing traefik
/usr/bin/sandbox-exec -f /private/tmp/homebrew20210819-12914-1pyb6c8.sb ruby -W1 -- /usr/local/Homebrew/Library/Homebrew/test.rb /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/traefik.rb --verbose --retry
INFO[0000] Configuration loaded from file: /private/tmp/traefik-test-20210819-12915-g8tcsm/traefik.toml
==> curl -sIm3 -XGET http://127.0.0.1:52417/
==> curl -sIm3 -XGET http://127.0.0.1:52416/dashboard/
==> /usr/local/Cellar/traefik/2.5.0/bin/traefik version 2>&1

which seems to be working.

  1. If the problems are only limited to ARM (not sure, as the linked PR is confusing on that aspect), which would explain why the test is working on my non-ARM laptop, then can you tell us what exactly is failing on ARM? it is true that we do not run any tests for ARM builds, but at least we provide binaries for all architectures, so we do know that at least traefik 2.5 is building on ARM.

@roopakv
Copy link
Contributor Author

roopakv commented Aug 19, 2021

@mpl

Take a look at this: golang/go#45702

Basically with go 1.17 traefik will not build unless x/sys is upgraded

I can write up something a bit more detailed in a bit :)

@ldez
Copy link
Contributor

ldez commented Aug 19, 2021

@roopakv currently traefik (v2.5.0) is built with go1.17 and all our CI use go1.17

@roopakv
Copy link
Contributor Author

roopakv commented Aug 19, 2021

@ldez interesting. Let me see why the arm bottle failed again. I was sure that all failures I collected were the x/sys issue.

@mpl
Copy link
Collaborator

mpl commented Aug 19, 2021

@ldez interesting. Let me see why the arm bottle failed again. I was sure that all failures I collected were the x/sys issue.

Yeah, in my point 3. above, it was implicit but I meant we are building traefik 2.5, for ARM, with go 1.17.

@thaJeztah
Copy link

If someone knows which commit in x/sys fixes the issue, then people could check if the version they use is has that commit; do we know which specific commit fixes the issue?

@roopakv
Copy link
Contributor Author

roopakv commented Aug 22, 2021

@mpl @thaJeztah aand @ldez from the logs on the golang brew upgrade:
image

this is definitely the x/sys issue. Upgrading x/sys as mentioned here: golang/go#46763
and golang/go#45702 will unblock building traefik with go 1.17 and unblock homebrew as well.

@ldez
Copy link
Contributor

ldez commented Aug 23, 2021

Can you provide the hash or the tag of traefik that you are trying to build?

@roopakv
Copy link
Contributor Author

roopakv commented Aug 23, 2021

531a8ff24843bcd903c195dcf418dbb0eb5c8e0d with go 1.17

@ldez
Copy link
Contributor

ldez commented Aug 23, 2021

As suggested by @thaJeztah, could you point the exact commit in x/sys that fixes the problem?

@roopakv
Copy link
Contributor Author

roopakv commented Aug 23, 2021

@ldez this is the commit that fixes the problem in x/sys golang/sys@a76c4d0

@roopakv
Copy link
Contributor Author

roopakv commented Aug 25, 2021

@ldez @thaJeztah anything I can doo here? i think this PR will unblock brew, and would love to get this in

@thaJeztah
Copy link

I'm not a maintainer of this project, only noticed the replace rule, but changes look good to me. Some observations (not sure what the general flow is on this project);

  • I see there's a merge commit in the PR (don't know if this project prefers merge or rebase on PRs)
  • This PR targets the v2.5 branch (don't know if this project develops on release branches, then merge back to main/master or cherry picks fixes into release branches)

@ldez
Copy link
Contributor

ldez commented Aug 25, 2021

We tried with several environments (M1, ...) and we don't reproduce the problem, I think we already spend too much time on that.

As it's not a real problem for us to upgrade this lib, I will approve and merge this PR anyway.
By doing this, we are making an exception in our process because we don't reproduce the problem.
This PR should be used neither as an example nor as a support for other PRs.

Also, for now, we don't plan to create a release only for that fix, we will wait a bit to get more feedback on the current version.

For next time, it's important to provide clear and complete reproducible context.

@ldez ldez added the bot/light-review decreases the number of required LGTM from 3 to 1. label Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants