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

Remove SCons #2777

Merged
merged 1 commit into from
Dec 14, 2020
Merged

Remove SCons #2777

merged 1 commit into from
Dec 14, 2020

Conversation

Holzhaus
Copy link
Member

No description provided.

@Holzhaus Holzhaus added the build label May 10, 2020
@Be-ing Be-ing marked this pull request as draft May 10, 2020 22:16
@Be-ing
Copy link
Contributor

Be-ing commented May 10, 2020

Thanks. We can't merge this until we get CMake working on macOS and switch the build servers to CMake.

@Be-ing Be-ing added this to the 2.4.0 milestone May 10, 2020
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

I think most of the files in the debian folder need to remain.
We need to find out what is the standard way of creating a deb file making use of cmake and follow it.

@@ -1,277 +0,0 @@
mixxx (2.2.2-0ubuntu1) bionic; urgency=medium
Copy link
Member

Choose a reason for hiding this comment

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

We need most of the diebisn files to create the deb packages.
I know that there is some automation with cmake and pebuilder, but some files are required anyway.

To keep almost empty changelog is stupid anyway. I think we need to find a way to make our main changelog file debian compliment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Typical entry in the (RPMFusion) distribution changelog: "New upstream release". Otherwise only technical changes when bumping the .spec version.

This is a different kind of changelog.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not everyone uses a Debian system and the main changelog should stay agnostic.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can already build a DEB package via cpack -G DEB.

@uklotzde
Copy link
Contributor

I would like to enforce the removal of SCons:

  • Keeping the SCons build consistent and working requires work for all new features.
  • The SCons builds consume precious Travis computing time.
  • If we don't do it there will be no pressure to complete the remaining tasks until 2.4 beta is imminent.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 10, 2020

Unfortunately we still need someone to make CMake builds not crash on startup on macOS...

@uklotzde
Copy link
Contributor

Unfortunately we still need someone to make CMake builds not crash on startup on macOS...

No one seems to care as long as the SCons builds are available. That's my point.

@daschuer
Copy link
Member

No one seems to care as long as the SCons builds are available. That's my point.

But if there is simply no one we are running into dead end with no MacOs support. I think we should find a contributor fro MacOS CMake first.

@uklotzde
Copy link
Contributor

Unfortunately finding volunteers for fixing macOS issues didn't work well in the past. I don't expect anyone will step in until we cut off this convenient workaround. In the worst case just a few weeks without nightly macOS builds.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 10, 2020

Considering we have a usable 2.3 beta branch with macOS builds now, this may be a good time to do that.

@daschuer
Copy link
Member

@crisclacerda has tried it unsuccessfully, he is now at Linux. But we will loose him entirely as master tester if we merge this without a working CMake.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 10, 2020

I am afraid breaking master with macOS will have the opposite effect and discourage macOS contributors.

@ywwg
Copy link
Member

ywwg commented Jul 10, 2020

Do we have an OS requirement for mac? I have an old (2009) Mac Pro that can run Mixxx, but I think the nvidia graphics card isn't supported in the newest OS

@Be-ing
Copy link
Contributor

Be-ing commented Jul 10, 2020

I think macOS 10.13 is our current minimum.

@ywwg
Copy link
Member

ywwg commented Jul 10, 2020

it looks like that's the last OS that supports nvidia so I might be able to make it work. I'll see if I can get it up and running

@Be-ing
Copy link
Contributor

Be-ing commented Jul 10, 2020

That would be great. Although I presume it will be painfully slow to keep retrying failing builds on a computer from 2009.

@ywwg
Copy link
Member

ywwg commented Jul 10, 2020

it was a beefy machine in 2009 capable of realtime hi rez video editing, so it should still be able to chew through gcc easily

@jcelerier
Copy link

Hi, I'm willing to test this on macOS - can someone explain to me what exactly is the test procedure ? should I just get this PR, and check that it builds with cmake ?

@Be-ing
Copy link
Contributor

Be-ing commented Jul 11, 2020

@jcelerier thanks for your interest in helping. Please read the prior Zulip discussion we had about the problem. There is a crash on startup in libsqlite3.dylib. I suspect this has to do with how CMake is linking Mixxx to SQLite and the custom collate function we use with SQLite.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 11, 2020

@jcelerier I see you also have a lot of experience with QML and you've ported RtMidi to C++17. We are currently brainstorming a new JavaScript controller mapping system including how we could use QML for controller screens. If you're interested in helping with that, take a look at this wiki page summarizing the current plans and the Zulip discussions linked from that page.

@Holzhaus
Copy link
Member Author

@jcelerier You don't need to test this PR (it just deletes SCons), just try to compile the master branch using CMake on MacOS and check if it's usable. Apparently it crashes on MacOS, but I can't debug it without access to a Mac.

@xeruf xeruf mentioned this pull request Aug 4, 2020
@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Oct 19, 2020
@Be-ing Be-ing changed the base branch from master to main October 23, 2020 23:23
@Be-ing
Copy link
Contributor

Be-ing commented Nov 10, 2020

Now that CMake packaging and code signing is working on macOS, I think we have one last obstacle before we can get rid of scons. Currently the PPA is updated by a Jenkins job running scons makeubuntu. Can we replace that with a script on Travis or GitHub Actions?

@Holzhaus Holzhaus marked this pull request as ready for review December 7, 2020 22:33
@Be-ing
Copy link
Contributor

Be-ing commented Dec 7, 2020

Yes, macOS builds, packaging, code signing, and deployment are all working with CMake now.

I think it might be easier to start this over on the 2.3 branch than resolve conflicts.

@daschuer
Copy link
Member

daschuer commented Dec 7, 2020

This cannot merged, because the debian folder needs to remain.
Please wait until this is cmake-ified.

How should the new directory layout look like?
cmake/debian
Originally I thought about rename the build folder to packaging or something. This can than hold also the RPM and Arch scripts.

@Holzhaus
Copy link
Member Author

Holzhaus commented Dec 7, 2020

I'm rebasing on 2.3 and take care of the issues.

@Be-ing
Copy link
Contributor

Be-ing commented Dec 7, 2020

Originally I thought about rename the build folder to packaging or something.

I like the idea to not put everything in the cmake folder. packaging would work well I think. We could move the build environment setup scripts from tools to packaging.

@daschuer
Copy link
Member

daschuer commented Dec 7, 2020

I'm rebasing on 2.3 and take care of the issues.

Please hold on until my PPA branch is ready. Otherwise this will conflict again.

@Holzhaus Holzhaus changed the base branch from main to 2.3 December 7, 2020 22:59
@Holzhaus
Copy link
Member Author

Holzhaus commented Dec 7, 2020

Originally I thought about rename the build folder to packaging or something.

I like the idea to not put everything in the cmake folder. packaging would work well I think. We could move the build environment setup scripts from tools to packaging.

Not sure about this. I think we already have enough top-level directories, and the setup scripts are not related to packaging. They are also needed for macos/windows users that just want to build Mixxx locally.

@Holzhaus
Copy link
Member Author

Holzhaus commented Dec 8, 2020

@daschuer i moved the Debian directory to cmake/ instead of removing it. The merge conflicts should be neglible then, right?

@daschuer
Copy link
Member

For my understanding, the cmake folder contains cmake helper files, that are as good as possible project independent.
If we move the individual packaging files there as well, it can be supprising for contributors.

Setup script at for my understanding a part of the package. IMHO In all cases, you have the binary, additional data, metadata and setup scripts that are more or less packaged to one achieve.

@Holzhaus
Copy link
Member Author

I meant the buildenv setup scripts that not supposed to be packaged. They are in the tools/ directory where they belong, because they are helper scripts for developers/testers to make building mixxx easier.

@Be-ing
Copy link
Contributor

Be-ing commented Dec 12, 2020

Finally, let's do this. @Holzhaus there are some conflicts now.

@Holzhaus
Copy link
Member Author

Did I mess up the merge? Something is wrong with the debian source packaging.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

I have no idea why the source package export fails. The error message is not very helpful.

I suggest to start this over by rebasing.

.github/workflows/build.yml Outdated Show resolved Hide resolved
@@ -182,7 +182,7 @@ jobs:
${{ matrix.os }}-${{ matrix.compiler_cache }}

- name: "Create build directory"
run: mkdir cmake_build
run: mkdir build
Copy link
Member

Choose a reason for hiding this comment

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

Do all jobs have a new working directory?
I know an approach where you use subdirectories for different builds.
I this relevant here.
I use for instance a build/Debug folder and a build/RelWithDebugInfo folder.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do all jobs have a new working directory?

On GitHub Actions, yes.

I know an approach where you use subdirectories for different builds.
I this relevant here.
I use for instance a build/Debug folder and a build/RelWithDebugInfo folder

Every build starts in a clean container, so there is no reason to use different directories on GitHub Actions.

Copy link
Member

Choose a reason for hiding this comment

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

Ok.

That's might be the issue for the packaging fail. Cpack alown fails.
You need to configure it with cmake first.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for the packaging fail was that the PPA step still used the cmake_build directory instead of build, but since that step was new it didn't caused a conflict. I amended the merge commit.

CMakeLists.txt Outdated Show resolved Hide resolved
@@ -1,66 +0,0 @@

Transifex doesn't handle wxl files.
Copy link
Member

Choose a reason for hiding this comment

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

I think we still need these files, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, localized WIX installers are a total hack and I wasted way too much time to make them work with CPack. If nobody volunteers to implement it, let's remove them. We could just pull them from transifex if somebody wants to add them, right?

Copy link
Member

Choose a reason for hiding this comment

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

What is the current situation?
Scons = localized wix
Cmake = english only?

cmake/debian/mixxx-test.install Outdated Show resolved Hide resolved
@daschuer
Copy link
Member

Can you rebase / squash commits to avoid double renamings In the commit history?

- Move packaging files from packaging/ directory
- Tidy up cmake/ directory
- Remove obsolete files from the build directory
@Holzhaus
Copy link
Member Author

Before merging, let's prepare a blog post, as this will probably break the workflow for a bunch of third party package maintainers that still use SCons (e. g. the mixxx-git package on the AUR). I'll write one this evening.

@Be-ing
Copy link
Contributor

Be-ing commented Dec 14, 2020

You can discuss moving to GitHub Actions from our old build servers in that post too.

@uklotzde
Copy link
Contributor

Before merging, let's prepare a blog post, as this will probably break the workflow for a bunch of third party package maintainers that still use SCons (e. g. the mixxx-git package on the AUR). I'll write one this evening.

Already happened: https://bugs.launchpad.net/mixxx/+bug/1907528

@Be-ing
Copy link
Contributor

Be-ing commented Dec 14, 2020

Good point. SCons is already broken. Merge now before waiting for the blog post?

@uklotzde
Copy link
Contributor

Good point. SCons is already broken. Merge now before waiting for the blog post?

No need to wait. It is already broken and main does not build with SCons.

@Be-ing Be-ing merged commit 13fd25a into mixxxdj:2.3 Dec 14, 2020
@Be-ing
Copy link
Contributor

Be-ing commented Dec 14, 2020

We still need to update the Windows build documentation.

@Holzhaus
Copy link
Member Author

mixxxdj/website#171

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants