Skip to content
This repository was archived by the owner on Mar 3, 2020. It is now read-only.

Include the code that we need#742

Closed
mike-burns wants to merge 4 commits into
masterfrom
include-things
Closed

Include the code that we need#742
mike-burns wants to merge 4 commits into
masterfrom
include-things

Conversation

@mike-burns
Copy link
Copy Markdown
Contributor

  • cin and cout require iostream.
  • QT_VERSION requires QtGlobal.
  • QSocketNotifier requires QSocketNotifier.
  • Unrelated to includes: let Qt figure out the spec.

Found by Jeremy Evans on behalf of the OpenBSD project.

- `cin` and `cout` require iostream.
- `QT_VERSION` requires QtGlobal.
- `QSocketNotifier` requires QSocketNotifier.
- Unrelated to includes: let Qt figure out the spec.

Found by Jeremy Evans on behalf of the OpenBSD project.
Comment thread src/StdinNotifier.h Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's #include <QSocketNotifier> in the .cpp file instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, that also works.

@mhoran
Copy link
Copy Markdown
Collaborator

mhoran commented Mar 26, 2015

@jferris (or anyone on a Mac) -- could you test this out? We removed the -spec argument from qmake as it does not support additional platforms without modification. Also, qmake seems to be able to figure out the spec on its own, and on OS X correctly detects clang instead of GCC.

I had tested this previously (on the breakpad branch) but never merged it in. I would expect things to be fine but I do not know the history of why the spec was set manually. It seems to have stuck around from when it was originally added to the Rakefile, and later moved to CapybaraWebkitBuilder.

@jferris
Copy link
Copy Markdown
Contributor

jferris commented Mar 27, 2015

(or anyone on a Mac)

I'm not on a Mac anymore.

I do not know the history of why the spec was set manually.

When I first started the project, it didn't compile correctly on OS X without the spec. I forget what the exact error was. This may have been improved since the early Qt versions we started on (maybe 4.6).

@nritholtz
Copy link
Copy Markdown
Contributor

Hey @mhoran. If you can tell me the steps of what I need to do, I can test it out on my Mac laptop.

@mhoran
Copy link
Copy Markdown
Collaborator

mhoran commented Mar 27, 2015

Thanks @nritholtz! Check out the include-things branch. All that needs to be checked is whether this branch still compiles on a Mac. rake build should be sufficient.

@nritholtz
Copy link
Copy Markdown
Contributor

@mhoran Here is gist with output after checking out include-things branch and running rake build.

Please let me know if there is anything I can do to help.

@mhoran
Copy link
Copy Markdown
Collaborator

mhoran commented Mar 27, 2015

It looks like it was successful, despite the warning. Not sure where that's coming from. If you try to run the specs (bundle && rake), do they pass? (I know you have a separate issue for the compatibility specs).

Thanks!

@nritholtz
Copy link
Copy Markdown
Contributor

Here's the bundle && rake results.
The only spec I see that failed is the selenium one, which I know is failing due to me having a newer version of Firefox. I have a PR, #740, which bumps the version to support latest Firefox versions, which should have this spec pass.

@mhoran
Copy link
Copy Markdown
Collaborator

mhoran commented Mar 27, 2015

Awesome, thanks!

@mhoran
Copy link
Copy Markdown
Collaborator

mhoran commented Mar 27, 2015

Merged as 0506f24.

@mhoran mhoran closed this Mar 27, 2015
@mhoran mhoran deleted the include-things branch March 27, 2015 20:37
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.

4 participants