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

3.7win without IPC fix #1852

Merged
merged 35 commits into from
Feb 21, 2016
Merged

3.7win without IPC fix #1852

merged 35 commits into from
Feb 21, 2016

Conversation

bagong
Copy link
Contributor

@bagong bagong commented Feb 4, 2016

Consider this a discussion base. It basically contains most from the old Winport branch except Lucas Cornelisses IPC fix. So it's a Win version that builds but has the bug.

I added:

  • Miguel's open long files PR.
  • tims commit containing most of Lucas' stuff for VS64bit compatibility (I "cherry-pick"ed that single commit from master. Not sure what else that brought in that wasn't planned to be in the 3.7 branch)
  • other smaller stuff from Lucas' port that allow the VS build to succeed (like an alternative to sys/time.h and some small Qt-related stuff
  • cmake changes/additions that make building with VS and msys simple
  • a README that is in sync with those cmake changes

99% of this is purely Windows related and doesn't effect the other OSes.

In theory somebody could try to take Lucas' IPC fix and apply it on a branch that branches from this release branch. That would allow for a Windows version that is still fairly close to 3.7, and the other OSes wouldn't be affected by the IPC fix... Unfortunately I cannot do this myself, but merging this would provide a useful base...

@crucialfelix crucialfelix added this to the 3.7.0 milestone Feb 4, 2016
@crucialfelix
Copy link
Member

Awesome !

@@ -24,6 +24,19 @@ if (${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_BINARY_DIR})
message(WARNING "WARNING: IN-PLACE BUILDS ARE NOT RECOMMENDED - PLEASE USE A BUILD DIRECTORY")
endif()

# allows cmake-find modules and Windows installer to identify the right architecture
if(WIN32)
if(CMAKE_GENERATOR MATCHES "64" OR "$ENV{MSYSTEM}" MATCHES "64")
Copy link
Contributor

Choose a reason for hiding this comment

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

CMAKE_CL_64 to detect 64bit msvc.

@bagong
Copy link
Contributor Author

bagong commented Feb 14, 2016

The Travis error is the same one as before...

@bagong
Copy link
Contributor Author

bagong commented Feb 18, 2016

For some reason Travis hasn't started building that last commit. But according to Winmerge this branch is now identical with the testbranch I was building up... And there it builds.. Don't know how to trigger the build manually

@bagong
Copy link
Contributor Author

bagong commented Feb 18, 2016

I am done with this. It looks as Travis didn't build the last commit, but as far as I can see, it did, only it seems to be out of sync with my commits. The ad8d9dc (second last) commit is marked okay now, although that was marked failed when it was initially triggered. I did try to retrigger the pull-request manually and it stayed on the second last commit but succeeded. What I think happened is that it compiled the last commit but marked the second last commit. The last commit, according to winmerge is exactly identical with the last commit on the testTravis fake PR I made to see when the error actually comes up. It seems, the error is triggered when in cmake path expressions that contain variables are not wrapped in string delimiters. Not in every case, but in the ones I corrected. That python test is completely intransparent...
So I think I met all request by Tim

  • except the "feed my grandma cast problem", which I put in OS check if's to use it only when we compile with VS: bagong@39a766a
  • The Q_OS_XXX thing (bagong@0418264) required to reformulate the ifdefs as suggested by Tim. Apparently that's a Qt thing, I saw it somewhere on stackoverflow. I also include QtGlobal. Including QtGlobal only (without the syntactical adjustment) actually made the build break for MinGW too at this place, so it may be that this construction was simply ignored on some platforms.
  • The biggest thing is the portaudio integration. Note that this is completely ignored on other platforms than Windows. I tested on both Linux and Apple that their build system is not contaminated when building with portaudio. My consideration for the inclusion of PA: on Windows you cannot get binaries of PA. Furthermore it is really difficult to compile PA using cmake if you want to get anything more than the basic WMME support. The only way to build PA fairly easily on Windows with other sound-api's is using msys and the unix build tools. That again is a different computer culture which I now tried to make unnecessary for building SC on Windows. What happens with this setup is: if a user does not provide PS libraries, SC will build a basic version as part of the SC build. If he/she does provide a library, that library will be preferred (can static or dynamic). Something I didn't manage is to allow the in place/embedded build of pa compile anything else than WMME support, but: the pa source is patched in a way that if he or she build PA separately from th SC build, the cmake pa build is going to succeed for all audio API's except WASAPI (which is excluded for MinGW in the options). I puzzled together patches from 3 different sources until it succeeded. You can see the patches in the submodule, it's a git repo and the patches are commits. I am happy to move that git repo to some neutral place. It git svn's the portaudio svn repo and is at latest pa-svn
  • remains the master commits that came in with the big Tim commit which I cherry-picked from master. In my opinion we shouldn't be too formalistic: these are fixes of typos in documentation, right. What bad can come from them in the 3.7 branch? So the only relevant one is Dan's bagong@a630e05. Should I revert it, or is it actually good to have it in 3.7?

best
.r.

@bagong
Copy link
Contributor Author

bagong commented Feb 19, 2016

Lucas' IPC fix is also done. It's a single commit on top of this, nicely separated. So with that commit Windows is stable, remains MIDI and the interactive console from the obvious things...

@bagong
Copy link
Contributor Author

bagong commented Feb 21, 2016

Great! ;)

@crucialfelix
Copy link
Member

ok, now done ? (you added a few commits yesterday)

As you say, the big thing is adding portaudio in. The submodule will be there even for non-windows builds. This is a downside. Its your own fork, and that may causes stress/maintenance issues later. But at least we have windows running now, and the alternatives are worse. It can be easily switched for another (possibly an pa official) version in the future.

That read me file is pretty epic. I didn't read it all. In the end do Anna Krenina and Count Vronsky live happily ever after ?

We should merge this now, oder ? If you want to wait a day or two just to be sure, that's fine too.

You are a hero @bagong !

@bagong
Copy link
Contributor Author

bagong commented Feb 21, 2016

Thanks! Yep, merge ;)
As to portaudio: as I stated, I am happy to move the PA submodule to the supercollider repo, I would actually prefer that...

telephon added a commit that referenced this pull request Feb 21, 2016
@telephon telephon merged commit 3ed7345 into supercollider:3.7 Feb 21, 2016
@crucialfelix
Copy link
Member

If there is any need to change it or burden on you then we can move it into supercollider when that time comes.

@bagong
Copy link
Contributor Author

bagong commented Feb 21, 2016

okle

@bagong bagong deleted the 3.7win branch February 21, 2016 22:03
@bagong
Copy link
Contributor Author

bagong commented Feb 21, 2016

I have pushed Lucas' "IPCfix" that makes the Windows version stable as branch "topic/IPCwithQTcp. Depending on what happens until the release, it could be used to make the win release stable... I'll try to keep it on top of 3.7...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants