Skip to content
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

codequery: transition to qt5 #4258

Closed
wants to merge 1 commit into from
Closed

Conversation

JCount
Copy link
Contributor

@JCount JCount commented Aug 27, 2016

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same formula update/change?
  • Have you built your formula locally prior to submission with brew install <formula> (where <formula> is the name of the formula you're submitting)?
  • Does your submission pass brew audit --new-formula <formula> (after doing brew install <formula>)?

transition codequery to build using Qt 5


In its current state this is a bit rough, with a somewhat hard-coded path for qscintilla2's library in order for cmake to find it and use it.

This is due to the fact that codequery's FindQt5QScintilla.cmake looks for NAMES qt5scintilla2 qscintilla2-qt5 whereas the dyld homebrew builds is libqscintilla2.dylib.

If anyone can think of a way to do this more neatly I would love any suggestions/guidance.

I suppose that this is part of the larger effort being chronicled in #1705

(I hope the build bot doesn't choke on this for some strange reason)

EDIT:
this is just an idea I am throwing out there, and it may not be really practical, but qscintilla2's formula could simply be modified to create a qscintilla2-qt5.dyld sym since it is now only being built with qt5 after all.

@JCount
Copy link
Contributor Author

JCount commented Aug 27, 2016

ok... so it choked completely on yosemite's sipnot being able to execute python3 because the build system isn't resolving it's complete dependency tree. the problem seems to lie with the fact that the depends_on sip => "with-python3" is wrapped in a conditional within qscintilla2. Therefor the brew bot isn't recognizing that to build the bottle for the current version of this formula it has to build sip from scratch "with-python3". or something along those lines...

@JCount JCount force-pushed the codequery-qt5 branch 2 times, most recently from 216ae05 to 9d9e683 Compare August 27, 2016 01:35
@JCount
Copy link
Contributor Author

JCount commented Aug 27, 2016

Adding depends_on :python3 => :build seems to have allowed the brew bot to sidestep the the recursive dependency issue when building the bottles. Superficially the fact that python3 isn't directly used by the formula is a bit of an issue, however the entire recursive dependency tree currently means that anyone installing this would most likely already have python3 on their machine. (That is excluding the corner case where pyqt5 and qscintilla2 were built from source without-python3.) However, this really doesn't feel like the correct way to resolve this issue, since it adds a dependency to the formula seemingly only because of an issue with the brew bot.

There may be a better way of sidestepping the dependency issue, since it seems to only exist within the brew bot's testing/building workflow, but I currently have zero ideas, not even bad ones.

depends_on "qscintilla2"

def install
args = std_cmake_args
args << "-DBUILD_QT5=ON"
args << "-DQT5QSCINTILLA_LIBRARY=#{Formula["qscintilla2"].opt_prefix}/lib/libqscintilla2.dylib"
Copy link
Contributor

Choose a reason for hiding this comment

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

this is good, if you were asking for feedback on this

Copy link
Contributor

Choose a reason for hiding this comment

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

#{Formula["qscintilla2"].opt_prefix}/lib can be #{Formula["qscintilla2"].opt_lib} though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you @dunn, I completely overlooked that option

@tdsmith
Copy link
Contributor

tdsmith commented Aug 29, 2016

What's the error look like without the :python3 dependency?

The sip executable is the same whichever Python it's built against; I'm not sure if we're adding unnecessary complexity by asking for sip to be assembled with one python or another, but maybe that's a red herring here.

@JCount
Copy link
Contributor Author

JCount commented Aug 29, 2016

The current "fix" is definitely not the answer, but I put it in to see if everything else worked fine. As to the error, I think I will just force-push my original version so that the brew bot can show the error in its full glory, and therefore be more informative to you and anyone else.

@JCount JCount force-pushed the codequery-qt5 branch 2 times, most recently from e6a15be to 9264c81 Compare August 29, 2016 23:26
transition codequery to build using Qt 5
@JCount
Copy link
Contributor Author

JCount commented Aug 30, 2016

The basic problem seems to be that the brew bot pours the bottled sip which is built using python2 but then trying to install it using python3.


EDIT
qscintilla2.rb#L28 might be part of the problem since the fact that the conditional block enwrapping depends_on "sip" => "with-python3" has no else (only elsif)

@tdsmith
Copy link
Contributor

tdsmith commented Aug 30, 2016

Okay, the proximal cause of the failure is that brew install --only-dependencies --build-bottle --verbose codequery is failing.

That command installs sip correctly, like:

==> Installing codequery dependency: sip
==> Using the sandbox
/usr/bin/sandbox-exec -f /tmp/homebrew20160830-58433-y97sky.sb nice /System/Library/Frameworks/Ruby.framework/Versions/2.0/usr/bin/ruby -W0 -I /usr/local/Library/Homebrew -- /usr/local/Library/Homebrew/build.rb /usr/local/Library/Taps/homebrew/homebrew-core/Formula/sip.rb --verbose --with-python3

But python3 was never installed, so the sip build fails.

@ruben2020
Copy link
Contributor

I'm the owner of codequery and I will port it to qt5 for homebrew.

Even now, it can be built for qt5 using:
cmake -G "Unix Makefiles" -DBUILD_QT5=ON ..

However, the GUI executable is called codequery-qt5. I want to change that to codequery. So I need to release a new version.

@JCount
Copy link
Contributor Author

JCount commented Aug 31, 2016

@ruben2020 Thank you very much for the upstream help! I can close this PR if you would like to submit your own, or I could simply adapt this PR to your new release for qt5 support whenever it come out. Whichever one you would prefer is fine, and is what I will do. Thank you again!

@ilovezfs
Copy link
Contributor

ilovezfs commented Sep 1, 2016

@BrewTestBot test this please

@ilovezfs
Copy link
Contributor

ilovezfs commented Sep 1, 2016

@JCount I merged Homebrew/brew#835 so hopefully this will now pass CI.

@JCount
Copy link
Contributor Author

JCount commented Sep 1, 2016

@ilovezfs Your fix to formula_installer seems to have worked, since the CI is all green and python3 is now correctly being installed/satisfied before the brew bot tries to satisfy qscintilla2's depends_on "sip" => "with-python3". Good insight and work in addressing the actual problem rather than chasing red herrings the way I was! (You actually understand what the brew bot and CI are doing, must be one of the reasons you are a maintainer) 😉

@MikeMcQuaid
Copy link
Member

Thanks for your contribution to Homebrew! Without people like you submitting PRs we couldn't run this project. You rock!

@JCount JCount deleted the codequery-qt5 branch September 1, 2016 20:13
@ruben2020
Copy link
Contributor

Thanks @JCount and @ilovezfs

@ilovezfs
Copy link
Contributor

ilovezfs commented Sep 2, 2016

@ruben2020 you're welcome! thanks for being such an engaged and accessible upstream

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
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.

None yet

6 participants