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

[Re-pull-request] Build system - Replace *_build_externals and *-pre-copy-binaries bash scripts with CMake #3132

Merged
merged 14 commits into from
Jul 12, 2022

Conversation

SunderB
Copy link
Contributor

@SunderB SunderB commented Jul 11, 2022

A (hopefully) fixed version of #3081.

  • Run mix release after building the NIFs
  • Fix NIF file extensions on macOS
  • Update CI to use OS specific *-build-gui scripts

Fixes #3129.


Original description:

The overall build process should be unchanged, apart from that sp_link, sp_midi and aubio are now built when running cmake --build.

Some changes have been made to the CMakeLists of sp_link and sp_midi to add an install target. I'm happy to put these in upstream PRs if needed.

@lilyinstarlight
Copy link
Contributor

Oh nice the CI doesn't use the <platform>-build-gui scripts, so it's failing for this change. You might want to adjust CI to be calling those scripts instead of running cmake directly. Maybe the scripts should also possibly be called <platform>-post-tau-prod-release or something, since they aren't in the prebuild step anymore? (eventually would be nice to get it in CMake too, but need to propagate more options iirc)

@SunderB
Copy link
Contributor Author

SunderB commented Jul 11, 2022

(eventually would be nice to get it in CMake too, but need to propagate more options iirc)

That's (almost certainly) possible. I had a branch in which I tried to implement as much of the build process as possible in CMake, which is where the original PR came from. I managed to get most of the prebuild steps into CMake and I think it built sucessfully (excluding the issues this PR fixes): https://github.com/SunderB/sonic-pi/tree/patch/remove-build-scripts. I didn't want to make a PR for this branch as it would be too many changes at once, but maybe at some point we could break the remaining changes down, polish them, and apply them bit by bit.

@samaaron
Copy link
Collaborator

samaaron commented Jul 11, 2022

Just a quick heads up - the gui build script is something I use quite a lot just to build the GUI. The main motivation I had for separating the build scripts out like this is that I got to have more fine grained control of building specific parts of the app whilst developing. For example, I often want to build the GUI separate from rebuilding the Beam release.

This is something I really want to keep on being able to do - so the tau build script should probably be called from build-all and any future cmake plans need to provide some mechanism of replicating at least the granularity of the current scripts. If that's not possible, then I'd prefer to keep the scripts. I have no issue maintaining them and I'm yet to see a really powerful motivation for moving everything to cmake. Happy to be convinced otherwise though.

@samaaron
Copy link
Collaborator

samaaron commented Jul 12, 2022

@SunderB thanks for moving the tau build out of the gui build script. However, it looks like it's now only called as part of the CI process and not part of the build-all dev build process.

I think we're pretty much already there, but I'm really keen on getting v4.0.1 out as it fixes a rather nasty boot issue for users with non-ASCII characters in their sound card names. I've therefore temporarily reverted this work. Once v4.0.1 is out (hopefully today) we can return to getting this properly working and committed again :-)

SunderB and others added 14 commits July 12, 2022 10:28
…th CMake

The overall build process is unchanged, apart from that sp_link, sp_midi
and aubio are now built when running cmake --build.
The default file extension for dynamic libs on macOS is '.dylib'.
However, Erlang expects '.so' on all UNIX-based platforms, so they need to
be renamed to be found by Erlang.
The scripts are now run after the main build (at the end of *-build-gui).
…linux-build-gui.sh

On Windows with MSVC, the build config is set at build time, while on
macOS and Linux using make, the build config is set at configure time.
@SunderB SunderB force-pushed the patch/2022-07-11-cmake-fix-tau-nifs branch from ede68d4 to 1680a10 Compare July 12, 2022 09:30
@SunderB SunderB changed the title Fix sp_midi and sp_link [Re-pull-request] Build system - Replace *_build_externals and *-pre-copy-binaries bash scripts with CMake Jul 12, 2022
@SunderB SunderB changed the title [Re-pull-request] Build system - Replace *_build_externals and *-pre-copy-binaries bash scripts with CMake [Re-pull-request] Build system - Replace *_build_externals and *-pre-copy-binaries bash scripts with CMake Jul 12, 2022
@SunderB
Copy link
Contributor Author

SunderB commented Jul 12, 2022

I've rebased the branch and added all the commits from #3081.

@SunderB
Copy link
Contributor Author

SunderB commented Jul 12, 2022

This is something I really want to keep on being able to do - so the tau build script should probably be called from build-all and any future cmake plans need to provide some mechanism of replicating at least the granularity of the current scripts. If that's not possible, then I'd prefer to keep the scripts.

I think that should be possible:

  • Use the add_custom_target command to make a 'tau' target
  • Make sp_midi and sp_link dependencies of Tau: add_dependencies(tau sp_midi sp_link)
  • To build Tau & dependencies alone, do: cmake --build . --target tau
  • To build the GUI alone, do: cmake --build . --target sonic-pi

I'm yet to see a really powerful motivation for moving everything to cmake. Happy to be convinced otherwise though.

In my opinion there's a few good reasons why it might be good to eventually move to CMake fully:

  • Ease of use - Makes it easier for new developers to build Sonic Pi using the same process as other projects using CMake.
  • Ease of maintenance - Having all the build logic in one set of scripts may help to minimise the duplication of build logic across the different platforms and the potential for accidental differences between scripts.
  • Installation - It may be possible to create installation targets for all of Sonic Pi so users building the latest version can install it easily.
  • Packaging - CPack allows for the creation of Windows WIX-based installers, Linux .deb and .rpm packages, and macOS bundles. In particular, this may make it easier for other package maintainers to maintain Linux packages.

But I also fully understand that you don't want to break things when they're working fine as they are. I'm not the most experienced, and I haven't worked with CMake much beyond the work I've done on Sonic Pi, so there may be reasons that I don't know of as to why going fully CMake may not be the best idea. And there may be some challenges with implementing everything, given the size and varied-ness of the project.

And, to be honest, anything is better than those old distro specific build scripts... :P

@SunderB
Copy link
Contributor Author

SunderB commented Jul 12, 2022

This PR is functionally done as far as I'm concerned - just needs testing now.

@lilyinstarlight
Copy link
Contributor

Yeah going fully CMake was not exactly my vision, just getting all of the major build points in it so code will build on more platforms. I'll echo most of the above points by @SunderB and also add that it can make it more accessible/familiar to potential contributors and that it would reduce support requirements, since the scripts have been causing problems on a variety of people's systems (hence a lot of my PRs being to fix or augment stuff in those scripts)

Breaking the changes into pieces like this and evaluating those along the way is definitely the least intrusive way to handle a bit of unification. And of course we'll only want stuff you're happy with too @samaaron, the last thing we want is to make anything more difficult for you :)

Also I would be completely unopposed for something better than CMake too because it's rather clunky and definitely not cut out for dealing with many non-C/C++ languages (maybe meson or something?)

Copy link
Contributor

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

I'll let @samaaron chime in, but as far as I can tell these changes are good now. I'm actually using this patch myself now because it simplifies my NixOS Sonic Pi package (which I'm using to get v4 into nixpkgs)

@samaaron samaaron merged commit 3313867 into sonic-pi-net:dev Jul 12, 2022
@samaaron
Copy link
Collaborator

Brilliant, thanks. Let's try integrating this work again :-)

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.

Timing problems post Merge pull request #3081 from SunderB/patch/2022-05-09-cmake-copy-bin…
3 participants