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

Engine Prime export uses libdjinterop snapshot API #3776

Merged
merged 1 commit into from
Apr 11, 2021

Conversation

mr-smidge
Copy link
Contributor

@mr-smidge mr-smidge commented Apr 10, 2021

This PR updates the Engine Prime export to use the 0.15.1 version of the libdjinterop library, that provides a new (breaking) API change allowing a track to be created or updated in a single call.

The new approach means that the number of database operations is greatly minimised, resulting in a much faster export experience.

As an additional bonus, the track rating is now exported too.

@mr-smidge mr-smidge marked this pull request as draft April 10, 2021 17:11
@Be-ing Be-ing requested a review from uklotzde April 10, 2021 17:18
@mr-smidge mr-smidge force-pushed the enh/djinterop-write-snapshot branch from cde1664 to 49e80c0 Compare April 10, 2021 17:48
@mr-smidge mr-smidge marked this pull request as ready for review April 10, 2021 22:08
@@ -1724,8 +1724,8 @@ if(ENGINEPRIME)
set(DJINTEROP_LIBRARY "lib/${CMAKE_STATIC_LIBRARY_PREFIX}djinterop${CMAKE_STATIC_LIBRARY_SUFFIX}")
file(MAKE_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/download")
ExternalProject_Add(libdjinterop
URL "https://github.com/xsco/libdjinterop/archive/0.14.6.tar.gz"
URL_HASH SHA256=db2f57f6c06c801d1785280ede0f032d7280bedd72f2a30bc263a272e3269587
URL "https://github.com/xsco/libdjinterop/archive/0.15.1.tar.gz"
Copy link
Contributor

@uklotzde uklotzde Apr 11, 2021

Choose a reason for hiding this comment

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

Out of scope: Did you consider switching to GIT_REPOSITORY + GIT_TAG? I Just used this variant recently in a project and it seems to be more concise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This did originally use GIT_REPOSITORY etc. but I changed it in a (sadly misguided) attempt to get the Ubuntu build working. I agree with you that GIT_REPOSITORY is more clear vs. a download link on GitHub. It's also easier during development to temporarily point to a branch (e.g. when I'm testing out a new version of libdjinterop that hasn't been merged yet).

I'll make a note to change this back at some point!

Copy link
Contributor

Choose a reason for hiding this comment

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

#3779

I wasn't aware of the Debian/Ubuntu build dependencies. The Fedora build system requires you to manually upload and hash sources in advance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a more full discussion in #3620

In short, there was no need to change from GIT_REPOSITORY to plain old http downloads. The Ubuntu build should not be downloading sources of dependencies from GitHub, and should instead be using packages. To that end, libdjinterop now has its own PPA, and I either Mixxx's PPA can use the same packages, or make a dependency on the libdjinterop PPA.

This reminds me to talk to Daniel about agreeing the approach, which I'll do on Zulip now.

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of this feature! LGTM

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.

2 participants