Skip to content

Make tidy-binaries find invocation work on Linux #27625

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

Merged
merged 2 commits into from
Aug 13, 2015
Merged

Conversation

wthrowe
Copy link
Contributor

@wthrowe wthrowe commented Aug 10, 2015

New enough find on Linux doesn't support "-perm +..." and suggests
using "-perm /..." instead, but that doesn't work on Windows.
Hopefully all platforms are happy with this expanded version.

I don't have access to a Windows development system to test this, so someone needs to verify that this actually works there before merging.

Closes #19981.

New enough find on Linux doesn't support "-perm +..." and suggests
using "-perm /..." instead, but that doesn't work on Windows.
Hopefully all platforms are happy with this expanded version.
@rust-highfive
Copy link
Contributor

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

This invocation is notoriously difficult to get right as I think this is probably the 10th PR related to trying to touch up these flags. Can you confirm:

  • Does this work on unix platforms? (e.g. linux/osx)
  • Does this actually find a binary if it's in-tree?

It... may be better to just rewrite this in python.

@wthrowe
Copy link
Contributor Author

wthrowe commented Aug 10, 2015

On Gentoo Linux:

rust $ make tidy-binaries
<snip>
check: binaries
rust $ cp x86_64-unknown-linux-gnu/stage1/bin/rustc src/test/
rust $ make tidy-binaries
<snip>
check: binaries
Binaries checked into src:
/home/wthrowe/computing/rust/src/test/rustc
/home/wthrowe/computing/rust/mk/tests.mk:269: recipe for target 'tidy-binaries' failed
make: *** [tidy-binaries] Error 123

I don't have rust checkouts on other machines to test, but I verified that when run on a directory foo containing:

-rwxr-xr-x 1 0 Aug 10 18:41 a
-rw-r-xr-- 1 0 Aug 10 18:41 g
-rw-r--r-- 1 0 Aug 10 18:41 none
-rw-r--r-x 1 0 Aug 10 18:41 o
-rwxr--r-- 1 0 Aug 10 18:41 u

(with a few irrelevant variations like g+w depending on machine) this output:

$ find foo -type f -perm -u+x | sort 
foo/a
foo/u
$ find foo -type f -perm -g+x | sort 
foo/a
foo/g
$ find foo -type f -perm -o+x | sort 
foo/a
foo/o
$ find foo -type f \( -perm -u+x -or -perm -g+x -or -perm -o+x \) | sort 
foo/a
foo/g
foo/o
foo/u

is the same on Gentoo, Ubuntu 14.04.2 LTS, Debian GNU/Linux 8.1 (jessie), Red Hat Enterprise Linux Server release 5.9 (Tikanga), Mac OS X 10.7.5, and FreeBSD 10.0.

It... may be better to just rewrite this in python.

That would also be reasonable, but I'm not very good with python so you don't want me doing it.

@alexcrichton
Copy link
Member

Ok, thanks! Let's see what bors has to say...

@bors: r+ f001f9a

Manishearth added a commit to Manishearth/rust that referenced this pull request Aug 11, 2015
New enough find on Linux doesn't support "-perm +..." and suggests
using "-perm /..." instead, but that doesn't work on Windows.
Hopefully all platforms are happy with this expanded version.

I don't have access to a Windows development system to test this, so someone needs to verify that this actually works there before merging.

Closes rust-lang#19981.
@bors
Copy link
Collaborator

bors commented Aug 12, 2015

⌛ Testing commit f001f9a with merge 170fb84...

@bors
Copy link
Collaborator

bors commented Aug 12, 2015

💔 Test failed - auto-win-gnu-32-opt

@wthrowe
Copy link
Contributor Author

wthrowe commented Aug 12, 2015

check: binaries
Binaries checked into src:
/c/bot/slave/auto-win-gnu-32-opt/build/src/test/pretty/issue-4264.pp
/c/bot/slave/auto-win-gnu-32-opt/build/mk/tests.mk:270: recipe for target 'tidy-binaries' failed

I bet the Windows build is unhappy with that file because it looks like it starts with a #! line. I'm not sure why that matches now but not with the old find invocation, but it seems reasonable to me to just add '*.pp' to the extension whitelist.

Pretty-printed files sometimes start with #![some_feature], which
looks like a shebang line and confuses Windows builds into thinking
they are executables.
@alexcrichton
Copy link
Member

@bors: r+ 904e428

@bors
Copy link
Collaborator

bors commented Aug 13, 2015

⌛ Testing commit 904e428 with merge 7b7fc67...

bors added a commit that referenced this pull request Aug 13, 2015
New enough find on Linux doesn't support "-perm +..." and suggests
using "-perm /..." instead, but that doesn't work on Windows.
Hopefully all platforms are happy with this expanded version.

I don't have access to a Windows development system to test this, so someone needs to verify that this actually works there before merging.

Closes #19981.
@bors bors merged commit 904e428 into rust-lang:master Aug 13, 2015
@wthrowe wthrowe deleted the find branch August 15, 2015 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants