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
tnftp 20070806 (new formula), tnftpd 20100324 (new formula), telnetd 54.50.1 (new formula), telnet and inetutils: conflicting files #19296
Conversation
Formula/telnet.rb
Outdated
@@ -4,6 +4,8 @@ class Telnet < Formula | |||
url "https://opensource.apple.com/tarballs/remote_cmds/remote_cmds-54.50.1.tar.gz" | |||
sha256 "156ddec946c81af1cbbad5cc6e601135245f7300d134a239cda45ff5efd75930" | |||
|
|||
conflicts_with "inetutils", because: "both install `telnet' binaries" |
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.
=>
remains the preferred syntax in core.
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.
@DomT4 Apologies, I couldn't get brew audit
to run properly.
homepage "https://opensource.apple.com/" | ||
url "https://opensource.apple.com/tarballs/remote_cmds/remote_cmds-54.50.1.tar.gz" | ||
sha256 "156ddec946c81af1cbbad5cc6e601135245f7300d134a239cda45ff5efd75930" | ||
|
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.
Probably want a keg_only :provided_pre_high_sierra
in this formula.
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.
@DomT4 Ok
Formula/telnetd.rb
Outdated
man8.install "telnetd.tproj/telnetd.8" | ||
end | ||
|
||
def caveats |
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.
Not sure any of these caveats are needed. We certainly shouldn't be telling people to try & touch /System
at least post-SIP.
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.
@DomT4 Ok, I'll remove them. Though, imho, SIP is overrated and I don't use it myself.
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.
Though, imho, SIP is overrated and I don't use it myself.
It protects standard users pretty well. It has certainly reduced the amount of problems Homebrew sees where people thought it was a brilliant idea to replace /usr/bin/clang
with a symlink to Homebrew's clang
, and so on.
Formula/tnftp.rb
Outdated
homepage "https://opensource.apple.com/" | ||
url "https://opensource.apple.com/tarballs/lukemftp/lukemftp-16.tar.gz" | ||
sha256 "ba35a8e3c2e524e5772e729f592ac0978f9027da2433753736e1eb1f1351ae9d" | ||
|
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.
Probably want a keg_only :provided_pre_high_sierra
in this formula.
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.
@DomT4 Ok
def install | ||
# Trying to use Apple's pre-supplied Makefile resulted | ||
# in headaches... they have made the build process | ||
# specifically for installing to /usr/bin and so it |
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.
Could always change that with inreplace
if it's not too "involved".
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.
@DomT4 I considered doing something like that, but I concluded that it probably would be too involved. Plus, what I have there already works so I don't see any real issue there.
Formula/tnftpd.rb
Outdated
depends_on :xcode => :build | ||
|
||
def install | ||
system "tar zxvf tnftpd-20100324.tar.gz" |
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.
Arguments should be separated by commas.
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.
@DomT4 Ok
Formula/tnftpd.rb
Outdated
|
||
def install | ||
system "tar zxvf tnftpd-20100324.tar.gz" | ||
Dir.chdir "tnftpd-20100324" |
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.
cd "blah" do
...
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.
@DomT4 As asked above, how is this preferable to Dir.chdir
?
Formula/tnftpd.rb
Outdated
system "./configure" | ||
system "make" | ||
|
||
File.rename "src/tnftpd", "src/ftpd" |
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.
These lines are all redundant. You can simply do things like:
bin.install "src/tnftpd" => "ftpd
etc
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.
@DomT4 Ok, thanks. Couldn't figure out how exactly to get that sort of thing to work.
Formula/tnftpd.rb
Outdated
etc.install "examples/ftpusers" | ||
end | ||
|
||
def caveats |
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.
IMO at best only caveat 2 is desirable here.
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.
@DomT4 Ok
Formula/tnftpd.rb
Outdated
EOS | ||
end | ||
|
||
test do |
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.
This should just work with an assert_match
, and again, you should use the full path, not simply "ftpd".
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.
@DomT4 As explained above with telnetd
, ftpd
doesn't seem to stick its output in the stdout. So that's why it's fiddling around with PTY
.
I'll fix it to use the full path.
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.
Try the 2>&1
dance here as well.
I'm pretty unconvinced personally all of these should be unique formula instead of being a broader utilities formula. |
@DomT4 I would be fine with that, in fact that was my initial idea. But I wasn't sure if that would be acceptable or not. |
@DomT4 Ok, sorry that I accidentally closed this ticket a little while ago. I was trying to update my fork and accidentally screwed things up with git and pushed some stuff I didn't mean to. I guess Github didn't like it and closed this ticket as a result. Anyway, I've tried to implement most of the changes that you wanted so please take a look at them and see if there's anything left. |
@DomT4 I've tried to fix everything that you've said to fix above. It seems that the test bot is now complaining about not being able to merge my fork properly -- I don't doubt that this is the result of me doing some awkward things with git to update my fork. How can I fix this so that the changes can get merged in properly? |
I'll look this over later today. Apologies for the delay, has been a pretty full-on week. |
@DomT4 No problem. Please let me know if I can do anything to help. |
@DomT4 Thanks for your further clarifications on my changes. Unfortunately, I've been having some really strange git problems and I can't really push or pull to my fork right now since it keeps wanting to rebase everything and then not being able to do so. I might have to put all of my changes on another branch or something. If you could fix any further issues and merge the changes, that would be fantastic as I don't really have time right now to dicker around with git and homebrew. |
I don't have the access rights to do so, no longer being a maintainer here. Will be something you need to look at yourself, I'm afraid. |
@DomT4 Alright, well, I suspect it will be several days, then. Hopefully I don't b0rk anything. |
Formula/telnetd.rb
Outdated
end | ||
|
||
test do | ||
# running a whole server, connecting, and so forth is a bit clunky and hard |
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.
Per an earlier comment:
What about if you use 2>&1 at the end of that shell_output line.
assert_match "usage: telnetd", shell_output("#{bin}/telnetd usage 2>&1")
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.
@DomT4 That does not work:
$ brew test telnetd
Testing telnetd
==> /usr/local/Cellar/telnetd/54.50.1/bin/telnetd usage 2>&1
Error: telnetd: failed
<0> expected but was
<1>.
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.
Off the top of my head, I don't think PTY
checks exit status, so it may well already be chucking out a bad one & it's simply not getting noticed. shell_output()
always checks exit status. You can tell it to expect a bad one, and as long as the assert_match
is still true it won't blow up.
assert_match "usage: telnetd", shell_output("#{bin}/telnetd usage 2>&1", 1)
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.
@DomT4 It is irrelevant if PTY
checks exit status. Running telnetd usage
already returns a non-zero exit status by default. Example:
$ telnetd usage
usage: telnetd [-debug] [-D (options|report|exercise|netdata|ptydata)]
[-h] [-k] [-l] [-n]
[-u utmp_hostname_length] [-U] [port]
$ echo $?
1
$
The only thing that matters in this test is that telnetd usage
returns the expected output. This is sufficient to determine whether the compilation succeeded.
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.
This is sufficient to determine whether the compilation succeeded.
Sure, but equally as a broad rule the simplest possible code is ideal in homebrew/core
because homebrew/core
is intended to be pretty friendly to people new to Ruby. The more code drifts from the most common usage the less likely people are to feel able to touch it in future, again, speaking very broadly.
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.
@DomT4 Sure, I understand that. I'm not sure how to change this code in order to be friendly like that while still making it functional, though...
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.
diff --git a/Formula/telnetd.rb b/Formula/telnetd.rb
index b27c1e4f41..ef313c4913 100644
--- a/Formula/telnetd.rb
+++ b/Formula/telnetd.rb
@@ -35,14 +35,6 @@ class Telnetd < Formula
end
test do
- # running a whole server, connecting, and so forth is a bit clunky and hard
- # to write properly so...
- require "pty"
- require "expect"
-
- PTY.spawn "#{bin}/telnetd usage" do |input, _output, _pid|
- str = input.expect(/usage: telnetd/)
- assert_match "usage: telnetd", str[0]
- end
+ assert_match "usage: telnetd", shell_output("#{bin}/telnetd usage 2>&1", 1)
end
end
Does the job. Tested locally.
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.
@DomT4 Ok, that works for telnetd
. I thought I recalled it not working for some reason, but I guess not. I'll apply your patch, although I tested the same sort of thing for tnftp
and tnftpd
and they did not work without PTY
.
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.
Yeah, that's expected. You're poking tnftp
with faux-interactivity, whereas this one is just doing a straight string read. Not sure what's going on with tnftpd
- It looks like it should work with this format of test too.
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.
@DomT4 Yes, I expected the same WRT tnftpd
. 🤷♂️
Formula/tnftpd.rb
Outdated
end | ||
|
||
def caveats | ||
<<-EOS.undent |
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.
This will need changing to the new <~EOS
style.
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.
@DomT4 Ok.
@DomT4 Ok, I've applied some of the changes you've suggested, as per our discussions. |
@DomT4 I'm getting a bizarre test failure message right now:
Do you know what this means I should do? I can't seem to get it to disappear. |
Formula/tnftpd.rb
Outdated
end | ||
|
||
def caveats | ||
<<~EOS.undent |
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.
It's complaining because the syntax is slightly wrong here. You want:
def caveats
<<~EOS
Blah
Blah
Blah
EOS
end
etc.
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.
@DomT4 Ok, thanks for clarifying that.
Formula/telnetd.rb
Outdated
"CFLAGS=$(CC_Flags) -isystembuild/Products/", | ||
"LDFLAGS=$(LD_Flags) -Lbuild/Products/" | ||
|
||
bin.install "telnetd.tproj/build/Products/telnetd" |
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.
bin
feels like a weird place for a daemon. I don't know. Homebrew has some mixed rules on this so don't move it based on my feelings alone unless @ilovezfs feels strongly.
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.
sbin is likely a better fit. does it need root to run?
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.
@ilovezfs Unfortunately, I've never actually used telnetd
... I just saw that some people complained about it being gone in 10.13 so I added it here. I don't know if it needs root privileges or not.
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.
@ilovezfs @DomT4 Ok, so all I needed to do to start telnetd
manually was to RTFM (from man 8 telnetd
):
The -debug option may be used to start up telnetd manually, instead of through inetd(8). If started up this way, port may be specified to run telnetd on an alternate TCP port number.
If the port number that telnetd
tries to bind to is below 1024 (which is the default) then we need root access. Though, we can pass a custom port number to -debug
which eliminates the need for root privileges. So I'm now not sure whether this should be in bin
or sbin
.
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.
Smells like sbin
to me, personally. Especially since we leave sshd
there.
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.
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.
ILZ gets to make the final call on that one. Personal opinion: Yes.
# in headaches... they have made the build process | ||
# specifically for installing to /usr/bin and so it | ||
# just doesn't play well with homebrew. | ||
|
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.
This empty line seems weird, given that you're continuing your sentence. I am curious how involved an inreplace
would be here.
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.
@DomT4 Sorry, I thought the empty line made sense. I can get rid of it if you want. Also, having examined Apple's makefile and having tried to fix it, it wouldn't be worth the trouble. Esp. since what we have already works...
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.
Reading the Makefile has left me with some questions. Particularly that the Makefile applies a bunch of patches, including one covering CVE-2014-8517, apparently. Is that something we're not getting by skipping the Makefile?
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.
@DomT4 Hmm, I missed that CVE patch. I guess patching Apple's makefile to patch the project may be worth it after all...
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 apply patches various ways, but yeah, we'll want to work out the most ideal way to apply any necessary patching but particularly any CVE-related patching since we shouldn't be merging new formulae vulnerable to years-old CVEs really 🙈.
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.
@DomT4 Never mind my previous comment. I checked the fetch.c
file and it already has the changes which would be applied in the CVE diff. The situation is the same with the other patches as well. I remember now, that is one of the reasons why I thought that bothering with Apple's Makefile wasn't worth it. I.e., the patches are already applied and so it is redundant to use their Makefile.
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.
@DomT4 In fact, when you use their Makefile, patch
asks you if you're trying to apply a reverse-patch. The reason being, obviously, that the changes are already applied. For example:
$ make
Installing source for tnftp...
/usr/bin/tar -C /tmp/tnftp/Sources -zxf /tmp/tnftp/Sources/tnftp-20070806.tar.gz
/bin/mv /tmp/tnftp/Sources/tnftp-20070806 /tmp/tnftp/Sources/tnftp
for patchfile in Makefile.in.patch PR-4074918.ftp.c.patch configure.patch PR-7577277.cmds.c.patch PR-7577277.extern.h.patch PR-7577277.fetch.c.patch PR-7577277.main.c.patch PR-7577277.util.c.patch PR-13253996_ftp.c.patch CVE-2014-8517.diff PR-24033140.main.c.patch; do \
(cd /tmp/tnftp/Sources/tnftp && patch -lp0 < /tmp/tnftp/Sources/patches/$patchfile) || exit 1; \
done
patching file src/Makefile.in
Reversed (or previously applied) patch detected! Assume -R? [n]
I apologise for the back & forth here. You picked one of the most involved types of initial PR anyone could file against Homebrew really. I appreciate that can be frustrating & appreciate the patience. |
Yes, evidently so... ;P I'm just glad that these tools will be available to people once again.
Well, thanks for the sympathy and for helping me thus far. |
@DomT4 Yes, that's pretty much what I'm talking about. I haven't verified that an older SDK will make it so that |
@ilovezfs @DomT4 @byss @ylluminate Just FYI for all those involved in this discussion: I've put up a post on StackExchange to help solve the problem with I'm rather tired of the immense tedium which has been involved in getting these formulas into homebrew so I hope that this issue with Thanks. |
@BrewTestBot test this please |
@ylluminarious the problem is likely that SDKROOT needs to be set. |
@ilovezfs Thanks for the suggestion. Unfortunately, I have already tried that but to no avail. As per another tip from somebody on StackExchange, I'm looking at the settings for the |
Try adding
|
yes that fixes it. I just tested directly on the Jenkins VM. |
@ilovezfs Well how about that! It does work! Thanks a lot for the advice. I'll go ahead and commit the change so that this error is fixed. Although, I have no idea how this fixed the problem; would you mind explaining it? |
There's only one SDK per Xcode version now. So if you're using Xcode 9 on 10.12, you often have to set deployment target to 10.12 or build goes 💥 Setting SDKROOT makes sure it uses Xcode headers and such instead of the CLT and is effectively the same as passing in isysroot. |
ca4484c
to
c4ff814
Compare
@ilovezfs Thank you for the explanation! I thought I had already set |
@ilovezfs Looks like we're good to merge... Anything else need to be done? |
both install 'telnet' binaries
@ylluminarious Nothing else needs to be done on your side. You've done a great job, especially as a first time contributor. 👍 😄 I currently am just cleaning up the git side of things a bit so that the commit history will be a little easier to read in the future. 😉 |
@JCount Thanks! I appreciate the compliment and the update. |
@ylluminarious I have a quick question. How did you derive the version numbers, (dates), for |
@JCount For |
create lukemftp alias
create lukemftpd alias
@ilovezfs Any thought on the version numbers? |
@ylluminarious Thank you for your first contribution to Homebrew, and especially for sticking with a prolongated PR! 🎊 💯 🎆 |
@JCount Thanks a lot! And thanks to everyone else who helped! |
Please pardon this interjection... As a proud father, I want to say I'm so grateful that you have such a strong work ethic and commitment to the the things you start. You always persevere and endure. You're a great young man, son and example to those who truly know you @ylluminarious. And thanks to everyone for your input, suggestions and help to him. |
@ylluminate Heh, thank you 😊 |
brew install --build-from-source <formula>
, where<formula>
is the name of the formula you're submitting?brew audit --strict <formula>
(after doingbrew install <formula>
)?I'm submitting these patches as a single pull request, since I committed these all at once to my fork and I don't know how to split them up into multiple pull requests. And I'm really tired. Apologies.
So, Apple removed some command-line tools for some important, albeit insecure, networking protocols in 10.13. In these patches, I've attempted to rectify some of those removals.
These patches do several different things:
First off, I've added a
conflicts_with
assertion forinetutils
intelnet.rb
. The reason is that both formulas install conflictingtelnet
binaries andtelnet
man pages. I see no easy way around that, so it's probably best to just say that they conflict with each other.I've added formulas for
tnftp
andtnftpd
. There is already an open pull request fortnftp
, but my formula uses a more recent version from Apple (20100324
as opposed to20091122
) and my formula actually works. Thetnftpd
formula is the same sort of thing, but installs theftpd
server program from Apple.I've added a formula for
telnetd
, which is just thetelnet
server on OS X.Now, there are some minor flaws with my formulas, or at least they are things which could be improved.
The first thing is that my tests for
telnetd
andtnftpd
either spit out usage information or show that the program is not completely brain-dead by making it recognize illegal options, respectively. The reason my tests are so simplistic is because writing tests which verify that servers are up and running is kind of difficult and, frankly, I don't want to put in the time to do it. Anyone else is more than welcome.The next thing is that it would be nice for the daemons' formulas to have
plist
methods so that users could use them viabrew services
. However, it seems that these daemons require root access to work properly. That makes sense, since that was how Apple intended for them to be used and they wrote theirplist
s accordingly. As such, though, I can't make use ofbrew services
since I would need to install theseplist
s to a folder owned by root, such as/Library/LaunchAgents
,/Library/LaunchDaemons
,/System/Library/LaunchAgents
, or/System/Library/LaunchDaemons
. As far as I can tell, homebrew offers no facilities to allow me to install any files into these locations.Finally, this is the first time I've contributed to homebrew so it's possible that there are other issues that I'm missing. Please point any such issues out or fix them yourself if you feel so inclined.