etcd: set ETCD_UNSUPPORTED_ARCH for Arm64, because while it builds on… #76995

areese · 2021-05-10T22:54:53Z

… Arm, it's not explicitly supported on m1 yet

Have you followed the guidelines for contributing?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

etcd isn't officially supported on m1, so this sets the flag for it.
Not sure how you want to handle this.

The upstream issue seems to be: etcd-io/etcd#10318

carlocab

Why do we need to set ETCD_UNSUPPORTED_ARCH?

carlocab · 2021-05-10T23:06:06Z

Formula/etcd.rb

@@ -42,6 +59,7 @@ def plist
          <key>Label</key>
          <string>#{plist_name}</string>
          <key>ProgramArguments</key>
+          #{plist_unsupported_platformenv}


Doesn't this break the plist, since the array [*] below is now a value with no key?

[*] This one:

<array> <string>#{opt_bin}/etcd</string> </array>

I think this also sets the environment variable for an entirely separate process, which is probably not what we want here.

The correct entry to add to the plist is probably (from man launchd.plist):

EnvironmentVariables <dictionary of strings> This optional key is used to specify additional environmental variables to be set before running the job. Each key in the dictionary is the name of an environment variable, with the corresponding value being a string repre- senting the desired value. NOTE: Values other than strings will be ignored.

Ugh I might have broken the plist.
Lemme see if I can figure that out.

I've not used plists much.
I'll fix it to add that instead.

areese · 2021-05-10T23:14:47Z

@carlocab etcd won't start without that var.
Here is brew test without:

% brew install etcd --build-from-source
==> Cloning https://github.com/etcd-io/etcd.git
Updating Library/Caches/Homebrew/etcd--git
==> Checking out tag v3.4.15
HEAD is now at aa71268 version: 3.4.15
HEAD is now at aa71268 version: 3.4.15
==> go mod vendor
==> go build -mod=vendor -ldflags -s -w -X main.version=3.4.15 -o /opt/homebrew/Cellar/etcd/3.4.15/bin/etcd
==> go build -mod=vendor -ldflags -s -w -X main.version=3.4.15 -o /opt/homebrew/Cellar/etcd/3.4.15/bin/etcdctl etcdctl/main.go
==> Caveats
To have launchd start etcd now and restart at login:
  brew services start etcd
Or, if you don't want/need a background service you can just run:
  etcd
==> Summary
🍺  /opt/homebrew/Cellar/etcd/3.4.15: 8 files, 36.6MB, built in 6 seconds
% brew test etcd                     
==> Testing etcd
etcd on unsupported platform without ETCD_UNSUPPORTED_ARCH=arm64 set
==> curl --silent -L http://127.0.0.1:2379/v2/keys/brew_test -XPUT -d value=Hello from brew test!
Last 15 lines from Library/Logs/Homebrew/etcd/test.01.curl:
2021-05-10 16:13:54 -0700

curl
--silent
-L
http://127.0.0.1:2379/v2/keys/brew_test
-XPUT
-d
value=Hello from brew test!

Error: etcd: failed
An exception occurred within a child process:
  BuildError: Failed executing: curl --silent -L http://127.0.0.1:2379/v2/keys/brew_test -XPUT -d value=Hello\ from\ brew\ test!
/opt/homebrew/Library/Homebrew/formula.rb:2163:in `block in system'
/opt/homebrew/Library/Homebrew/formula.rb:2099:in `open'
/opt/homebrew/Library/Homebrew/formula.rb:2099:in `system'
/opt/homebrew/Library/Taps/homebrew/homebrew-core/Formula/etcd.rb:70:in `block in <class:Etcd>'
/opt/homebrew/Library/Homebrew/formula.rb:1963:in `block (3 levels) in run_test'
/opt/homebrew/Library/Homebrew/utils.rb:558:in `with_env'
/opt/homebrew/Library/Homebrew/formula.rb:1962:in `block (2 levels) in run_test'
/opt/homebrew/Library/Homebrew/formula.rb:924:in `with_logging'
/opt/homebrew/Library/Homebrew/formula.rb:1961:in `block in run_test'
/opt/homebrew/Library/Homebrew/mktemp.rb:63:in `block in run'
/opt/homebrew/Library/Homebrew/mktemp.rb:63:in `chdir'
/opt/homebrew/Library/Homebrew/mktemp.rb:63:in `run'
/opt/homebrew/Library/Homebrew/formula.rb:2212:in `mktemp'
/opt/homebrew/Library/Homebrew/formula.rb:1955:in `run_test'
/opt/homebrew/Library/Homebrew/test.rb:43:in `block in <main>'
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/timeout.rb:93:in `block in timeout'
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/timeout.rb:33:in `block in catch'
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/timeout.rb:33:in `catch'
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/timeout.rb:33:in `catch'
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/timeout.rb:108:in `timeout'
/opt/homebrew/Library/Homebrew/test.rb:48:in `<main>'
%

areese · 2021-05-10T23:20:26Z

I'll rebase and commit with the fixed plist:

% plutil  -p ./Cellar/etcd/3.4.15/homebrew.mxcl.etcd.plist
{
  "EnvironmentVariables" => {
    "ETCD_UNSUPPORTED_ARCH" => "arm64"
  }
  "KeepAlive" => {
    "SuccessfulExit" => 0
  }
  "Label" => "homebrew.mxcl.etcd"
  "ProgramArguments" => [
    0 => "/opt/homebrew/opt/etcd/bin/etcd"
  ]
  "RunAtLoad" => 1
  "WorkingDirectory" => "/opt/homebrew/var"
}

% brew services start etcd

==> Successfully started `etcd` (label: homebrew.mxcl.etcd)

areese · 2021-05-10T23:21:05Z

@carlocab lemme know what you think of that. ty.

carlocab · 2021-05-10T23:23:58Z

The new service blocks might make conditionally setting environment variables a lot simpler here.

See, for example:

homebrew-core/Formula/php.rb

Lines 324 to 330 in c41973c

    
           service do 
        
             run [opt_sbin/"php-fpm", "--nodaemonize"] 
        
             run_type :immediate 
        
             keep_alive true 
        
             error_log_path var/"log/php-fpm.log" 
        
             working_dir var 
        
           end

or

homebrew-core/Formula/httpd.rb

Lines 129 to 133 in c41973c

    
           service do 
        
             run [opt_bin/"httpd", "-D", "FOREGROUND"] 
        
             environment_variables PATH: std_service_path_env 
        
             run_type :immediate 
        
           end

areese · 2021-05-10T23:39:48Z

That is far better, and it might fix the issue where it etcd complains.
If you spawn etcd directly it still gets upset, lemme take a shot at doing the service instead.

carlocab · 2021-05-10T23:41:59Z

I think you should just be able to do

environment_variables ETCD_UNSUPPORTED_ARCH: "arm64" if Hardware::CPU.arm?

in a service block.

carlocab · 2021-05-11T00:31:23Z

Formula/etcd.rb

+    keep_alive true
+    working_dir var
+  end
+
  def plist


You need to delete the plist method when using a service block.

lol I was wondering that ty.

Removed plist section
Tested out service and it seems to work:"

% /opt/homebrew/bin/etcdctl put foo bar OK % /opt/homebrew/bin/etcdctl get foo foo bar

actually, not happy with that.

The problem is, the way it is, we're not supporting arm at all, and I only looked specifically at arm on macOS.
updating to catch that specific.

Homebrew doesn't support arm in Linux so that would be fine like this

I’m still fighting with the service block
Last night it was complaining about plist
I’ll paste the error here soon

areese · 2021-05-11T00:31:46Z

@carlocab should the plist section stick around if adding a service?
98a2011

I left the plist section, but I think it should be removed looking at some of the other examples.
I need to test this out and make sure the service works.

… Arm, it's not explicitly supported on m1 yet Use a service block for etcd remove plist section

areese · 2021-05-11T00:37:27Z

@carlocab I think I've gotten somewhere with this.
lmk what you want to do.

% brew audit --strict etcd
%

areese · 2021-05-11T00:42:23Z

Correction, I don't think it's officially supported on MacOS arm64

This pr looks like it's supporting arm64, but as far as I can tell it's graviton
etcd-io/etcd#12929

Formula/etcd.rb

carlocab · 2021-05-11T14:26:12Z

Formula/etcd.rb

+      on_macos do
+        if Hardware::CPU.arm?
+          # etcd isn't officially supported on arm64
+          # https://github.com/etcd-io/etcd/issues/10318
+          # https://github.com/etcd-io/etcd/issues/10677
+          ENV["ETCD_UNSUPPORTED_ARCH"]="arm64"
+        end
+      end


If they don't support ARM at all, maybe we don't need this in an on_macos block.

They seem to be about to support arm on Linux

carlocab

Thanks, @areese. Will dispatch a bottle build separately.

areese · 2021-05-11T15:09:21Z

@carlocab This is what I was seeing last night that I don't fully understand:

 % brew reinstall --build-from-source etcd
==> Cloning https://github.com/etcd-io/etcd.git
Updating Library/Caches/Homebrew/etcd--git
==> Checking out tag v3.4.15
HEAD is now at aa71268 version: 3.4.15
HEAD is now at aa71268 version: 3.4.15
==> Reinstalling etcd 
==> go mod vendor
==> go build -mod=vendor -ldflags -s -w -X main.version=3.4.15 -o /opt/homebrew/Cellar/etcd/3.4.15/bin/etcd
==> go build -mod=vendor -ldflags -s -w -X main.version=3.4.15 -o /opt/homebrew/Cellar/etcd/3.4.15/bin/etcdctl etcdctl/main.go
Error: Failed to install plist file
🍺  /opt/homebrew/Cellar/etcd/3.4.15: 7 files, 36.6MB, built in 5 seconds
 % brew audit --strict etcd               
 %

The service is working fine.

carlocab · 2021-05-11T15:16:05Z

Oh, huh. So, to be sure, you see an error about the plist file, but are able to use the service anyway? That's weird.

CC @SMillerDev in case you know what's going on there.

carlocab · 2021-05-11T15:16:30Z

Bottled in a331500.

areese · 2021-05-11T15:21:00Z

Yeah that’s what confused me
I’m trying to set up a case sensitive install of macOS
But I’m going to poke it again

i had tested the service and ran audit and then rebuilt after adding the on macos and noticed this
It’s been there since I added the service block but I’ll poke around more

areese · 2021-05-11T16:15:06Z

I left this part of the patch on my desk some how, but given the conversation the only relevant part is
etcd-io/etcd#12929

I'm going to reproduce the plist complaint I had

diff --git a/Formula/etcd.rb b/Formula/etcd.rb
index 5df7ce5f653..8734516ca1f 100644
--- a/Formula/etcd.rb
+++ b/Formula/etcd.rb
@@ -29,7 +29,13 @@ class Etcd < Formula
   plist_options manual: "etcd"
 
   service do
-    environment_variables ETCD_UNSUPPORTED_ARCH: "arm64" if Hardware::CPU.arm?
+    # etcd isn't officially supported on MacOS arm64
+    # https://github.com/etcd-io/etcd/issues/10318
+    # https://github.com/etcd-io/etcd/issues/10677
+    # https://github.com/etcd-io/etcd/pull/12929
+    on_macos do
+      environment_variables ETCD_UNSUPPORTED_ARCH: "arm64" if Hardware::CPU.arm?
+    end
     run [opt_bin/"etcd"]
     run_type :immediate
     keep_alive true
@@ -44,6 +50,7 @@ class Etcd < Formula
           # etcd isn't officially supported on arm64
           # https://github.com/etcd-io/etcd/issues/10318
           # https://github.com/etcd-io/etcd/issues/10677
+          # https://github.com/etcd-io/etcd/pull/12929
           ENV["ETCD_UNSUPPORTED_ARCH"]="arm64"
         end
       end

carlocab · 2021-05-11T16:19:43Z

Hm, I'm not sure on_macos blocks work inside service blocks. That might be where the error is coming from. You can try to get more detail out of it with --verbose --debug.

areese · 2021-05-11T16:23:13Z

I bet that's what the problem is, as I just pulled master and don't see the error, and I don't have the on_macos in master.

BrewTestBot added go Go use is a significant feature of the PR or issue no ARM bottle Formula has no ARM bottle labels May 10, 2021

carlocab reviewed May 10, 2021

View reviewed changes

carlocab added the CI-force-arm [DEPRECATED] Don't pass --skip-unbottled-arm to brew test-bot. label May 10, 2021

areese force-pushed the m1-support/etcd branch from f6265b2 to 6084e80 Compare May 10, 2021 23:20

carlocab reviewed May 11, 2021

View reviewed changes

etcd: set ETCD_UNSUPPORTED_ARCH for Arm64, because while it builds on…

1e4f489

… Arm, it's not explicitly supported on m1 yet Use a service block for etcd remove plist section

areese force-pushed the m1-support/etcd branch from 98a2011 to 1e4f489 Compare May 11, 2021 00:34

carlocab reviewed May 11, 2021

View reviewed changes

Formula/etcd.rb Show resolved Hide resolved

carlocab reviewed May 11, 2021

View reviewed changes

carlocab approved these changes May 11, 2021

View reviewed changes

carlocab merged commit ba2212a into Homebrew:master May 11, 2021

areese deleted the m1-support/etcd branch May 11, 2021 15:09

github-actions bot added the outdated PR was locked due to age label Jun 11, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jun 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcd: set ETCD_UNSUPPORTED_ARCH for Arm64, because while it builds on… #76995

etcd: set ETCD_UNSUPPORTED_ARCH for Arm64, because while it builds on… #76995

areese commented May 10, 2021

carlocab left a comment

carlocab May 10, 2021 •

edited

Loading

areese May 10, 2021

areese May 10, 2021

areese commented May 10, 2021

areese commented May 10, 2021

areese commented May 10, 2021

carlocab commented May 10, 2021

areese commented May 10, 2021

carlocab commented May 10, 2021

carlocab May 11, 2021

areese May 11, 2021

areese May 11, 2021

areese May 11, 2021

SMillerDev May 11, 2021

areese May 11, 2021

areese commented May 11, 2021

areese commented May 11, 2021

areese commented May 11, 2021

carlocab May 11, 2021

areese May 11, 2021

carlocab left a comment

areese commented May 11, 2021

carlocab commented May 11, 2021

carlocab commented May 11, 2021

areese commented May 11, 2021

areese commented May 11, 2021

carlocab commented May 11, 2021

areese commented May 11, 2021

etcd: set ETCD_UNSUPPORTED_ARCH for Arm64, because while it builds on… #76995

etcd: set ETCD_UNSUPPORTED_ARCH for Arm64, because while it builds on… #76995

Conversation

areese commented May 10, 2021

carlocab left a comment

Choose a reason for hiding this comment

carlocab May 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

areese commented May 10, 2021

areese commented May 10, 2021

areese commented May 10, 2021

carlocab commented May 10, 2021

areese commented May 10, 2021

carlocab commented May 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

areese commented May 11, 2021

areese commented May 11, 2021

areese commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlocab left a comment

Choose a reason for hiding this comment

areese commented May 11, 2021

carlocab commented May 11, 2021

carlocab commented May 11, 2021

areese commented May 11, 2021

areese commented May 11, 2021

carlocab commented May 11, 2021

areese commented May 11, 2021

carlocab May 10, 2021 •

edited

Loading