-
-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
gensio 2.8.5 (new formula) ser2net: update to use gensio formula #169138
Conversation
On Tue, Apr 16, 2024 at 06:13:05AM -0700, Sean Molenaar wrote:
@SMillerDev commented on this pull request.
> @@ -0,0 +1,49 @@
+class Gensio < Formula
+ desc "Stream I/O Library"
+ homepage "https://github.com/cminyard/gensio"
+ url "https://downloads.sourceforge.net/project/ser2net/ser2net/gensio-2.8.4.tar.gz"
Is there no GitHub release?
There is a tag on GitHub, but traditionally I release the tarballs on
sourceforge because it's been there a long time and that's where
everyone expects it. I can add it to GitHub, if you like.
|
If the homepage is GitHub I would expect the code to also come from there. If there is a tag we can just download the tarball from there. |
On Tue, Apr 16, 2024 at 06:12:49AM -0700, Sean Molenaar wrote:
@SMillerDev commented on this pull request.
> system "./configure", "--disable-dependency-tracking",
"--prefix=#{prefix}",
```suggestion
system "./configure", *std_configure_args
```
Yes, that's better. That's in the ser2net formula, which I didn't
originally write, so I just didn't change it.
> + depends_on ***@***.***"
+ depends_on "portaudio"
+ depends_on ***@***.***"
+ depends_on "tcl-tk"
+ on_linux do
+ depends_on "alsa-lib"
+ depends_on "avahi"
+ depends_on "linux-pam"
+ end
+
+ def python3
+ "python3.11"
+ end
+
+ def install
+ # https://rubydoc.brew.sh/Formula.html#std_configure_args-instance_method
No need to document what parts of brew a formula is using
```suggestion
```
Ok, I removed the comment. I think that came from boilerplate, but I
don't remember.
> + depends_on "alsa-lib"
+ depends_on "avahi"
+ depends_on "linux-pam"
+ end
+
+ def python3
+ "python3.11"
+ end
+
+ def install
+ # https://rubydoc.brew.sh/Formula.html#std_configure_args-instance_method
+ system "./configure", *std_configure_args,
+ "--disable-silent-rules",
+ "--with-pythoninstall=#{lib}/gensio-python",
+ "--sysconfdir=#{etc}",
+ "--with-tclcflags=-I #{HOMEBREW_PREFIX}/include/tcl-tk"
Why not the macOS one?
I didn't think about that. It won't compile on Linux without doing
this, so I just used it for both. I'll fix that.
> + def python3
+ "python3.11"
+ end
Why not the latest one?
Oops, yes, that should be fixed.
Thank you,
…-corey
|
I have made all the requested modifications, I think it's ready for another review. |
Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request. |
a29e252
to
0aeb1c1
Compare
Ok, one more time. Hopefully I have everything, I don't see anything else. |
Anything else I need to do here? It's been three weeks without any real comments. |
The gensio library is a library to make stream I/O easy. It has all kinds of tools, including encryption, authentication. ser2net, a program for providing network access to serial ports, requires it. This adds the library as a new Forumla. Signed-off-by: Corey Minyard <[email protected]>
Now that gensio is a separate formula, remove all the build of gensio in ser2net and make it depend on gensio. Put the ser2net control directory in /opt/homebrew/etc/ser2net instead of being in the ser2net directory. Set the datarootdir, where ser2net stores it authentication information, to /opt/homebrew/share/ser2net. Run the service in forground as it should be. Don't enable the admin interface through the command line, the user can use ser2net.yaml to enable it if they want. Signed-off-by: Corey Minyard <[email protected]>
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.
we can followup on the audit failures
🤖 An automated task has requested bottles to be published to this PR. |
HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>
, where<formula>
is the name of the formula you're submitting?brew test <formula>
, where<formula>
is the name of the formula you're submitting?brew audit --strict <formula>
(after doingHOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>
)? If this is a new formula, does it passbrew audit --new <formula>
?I submitted this earlier, before gensio had enough likes and forks. It now does. All audits pass.
The ser2net formula builds gensio as part of its build, but gensio is a separate stand-alone library and set of tools. This commit splits gensio into its own separate formula so it can be properly maintained, and makes ser2net depend on that.
As well, the ser2net formula was non-functional. It had the following issues:
The configuration files were set to inside the /opt/homebrew/etc/ser2net, so every time you upgraded you would lose your configuration. This change moved it to /opt/homebrew/etc/ser2net.
Same problem with the authentication token location, that is moved to /opt/homebrew/share.
The service was run in the background, so that caused all kinds of confusion. This switches it to run in the foreground so brew can manage it.
The administration interface was enabled on the command line. This has security implications and is generally a bad idea outside of testing. The admin interface can be enabled in the configuration files with the proper authentication.