Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

audit fixes: D 01: dante - dex2jar #44621

Closed
wants to merge 1 commit into from
Closed

audit fixes: D 01: dante - dex2jar #44621

wants to merge 1 commit into from

Conversation

apjanke
Copy link
Contributor

@apjanke apjanke commented Oct 5, 2015

No description provided.

@@ -9,7 +9,7 @@ class Ddar < Formula

depends_on "xmltoman" => :build
depends_on :python if MacOS.version <= :snow_leopard
depends_on "protobuf" => "with-python"
depends_on "protobuf"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ brew audit ddar
ddar:
 * Dependency protobuf does not define option "with-python"

Since #39260 in 5/2015, protobuf now has a --without-python option; including it is the default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DomT4 Then what if this formula requires protobuf to be built with python but it isn't?

imac@iMacs-iMac /u/local> brew info protobuf
protobuf: stable 2.6.1 (bottled), devel 3.0.0-alpha-3.1
Protocol buffers (Google's data interchange format)
https://github.com/google/protobuf/
/usr/local/Cellar/protobuf/2.6.1 (81 files, 6.5M) *
  Built from source with: --without-python
From: https://github.com/Homebrew/homebrew/blob/master/Library/Formula/protobuf.rb
==> Options
--c++11
    Build using C++11 mode
--universal
    Build a universal binary
--with-check
    Run build-time check
--without-python
    Build without python support
--devel
    Install development version 3.0.0-alpha-3.1
==> Caveats
Editor support and examples have been installed to:
  /usr/local/Cellar/protobuf/2.6.1/share/doc/protobuf
imac@iMacs-iMac /u/local> brew info ddar
ddar: stable 1.0, HEAD
De-duplicating archiver
http://www.synctus.com/ddar/
/usr/local/Cellar/ddar/1.0_1 (20 files, 152K) *
  Built from source
From: https://github.com/Homebrew/homebrew/blob/master/Library/Formula/ddar.rb
==> Dependencies
Build: xmltoman ✔
Required: protobuf ✔

And after brew install --without-python, brew reinstall --with-python does not build with python.

imac@iMacs-iMac /u/local> brew reinstall protobuf --with-python
==> Reinstalling protobuf with --without-python
==> Downloading https://github.com/google/protobuf/releases/download/v2.6.1/protobuf-2.6.1.tar
Already downloaded: /Library/Caches/Homebrew/protobuf-2.6.1.tar.bz2
==> Patching
patching file src/google/protobuf/descriptor.h
==> ./configure --prefix=/usr/local/Cellar/protobuf/2.6.1 --with-zlib
^C

One another issue about ddar:

imac@iMacs-iMac /u/local> ddar
Traceback (most recent call last):
  File "/usr/local/Cellar/ddar/1.0_1/libexec/bin/ddar", line 20, in <module>
    import synctus.ddar_pb2, synctus.dds
ImportError: No module named synctus.ddar_pb2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we can express this properly with the current formula semantics. There doesn't seem to be a way of expressing "depends on X, where a particular option was not" used.

The --with-python option has no effect with brew install protobuf, since install just silently ignores any unrecognized options. When you do the reinstall, it remembers the previous --without-python. When you do a fresh install protobuf --with-python, it ignores --with-python and just goes with the defaults, which happens to be including python. To check, try using --with-miss-piggy instead of --with-python; it has the same effect. You can also do brew install protobuf --without-python --with-python; you'll end up without python.

Using an undefined option in the depends_on options list seems to have the effect of requiring the dependency to be built with the default options, which in this case works, and rebuilt from source every time instead of poured from a bottle. I think what happens is: since --with-python is undefined for protobuf, in install ddar, the dependency checker will check protobuf, see that with-python was not an option it was built with, do an installation of it using --with-python. The install step will ignore the unrecognized --with-python, so you end up with a build with the default options (which in this case does happen to include python support). Again, depends_on "protobuf" => "with-miss-piggy" has the same effect, so I think that's what's going on.

I looked through the source and doc, and it doesn't look like the formula code currently has a way of expressing a dependency on a formula option other than one which can be passed explicitly as an option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can reproduce that ImportError: No module named synctus.ddar_pb2 on some, but not all, of my machines. Don't know what's different about them; will look in to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ddar command has a #!/usr/bin/python line, so it runs with the system python. On my machine where ddar works, the system python has /usr/local/lib/python2.7/site-packages and some other /usr/local and Cellar locations on its site.path. (/usr/bin/python -c 'import sys; print "\n".join(sys.path)') On the system where ddar doesn't work, no /usr/local locations appear in sys.path for the system python.

I don't know what's causing the different paths on the two systems. On both, I have the Homebrew python installed, and it's first on my path.

Perhaps this is a side effect of having previously built some Homebrew python bindings against the system python in the past on one system but not the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yep, that's it: on the machine where this works, I have a ~/Library/Python/2.7/lib/python/site-packages/homebrew.pth, created per the Caveats you see when you install a formula with Python modules using the system Python. (e.g. PATH=/usr/bin:$PATH; brew install protobuf.) On the machine where it doesn't work, I have no such file.

What's the word on #! python paths for Homebrew-installed programs? I don't see anything in Homebrew and Python. The hardcoded #!/usr/bin/python makes ddar run against system python regardless of your $PATH settings or how it was built. ranger.rb seems to handle a similar issue with s.gsub! "#!/usr/bin/python", "#!#{PythonRequirement.new.which_python}"; maybe we should do that here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already an open PR on this for ddar: #43358

I'm removing ddar from this PR in favor of #43358.

@apjanke apjanke mentioned this pull request Oct 5, 2015
@DomT4
Copy link
Member

DomT4 commented Oct 7, 2015

Could you possibly drop devil into a different PR here?

It breaks pulling fresh bottles for the formulae in this PR because a bottle for devil isn't built because it needs gcc-4.9 to build; alas our pull code currently doesn't handle the idea you'd want to pull some things with and some things without a bottle from the same PR.

@apjanke
Copy link
Contributor Author

apjanke commented Oct 7, 2015

Sure. Removed devil.

@DomT4
Copy link
Member

DomT4 commented Oct 8, 2015

Merged in 9681dff. Thank you @apjanke!

@DomT4 DomT4 closed this in 9681dff Oct 8, 2015
@apjanke apjanke deleted the audit-fix-D-01 branch October 8, 2015 04:00
@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants