-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Use URI path for in-memory database #2511
Conversation
Out of a sudden various test started to fail in #2508 with SegFaults. After sorting out the dependencies between TrackCollectionManager and LibraryScanner and disabling the LibraryScanner during test runs those issues seem to be solved. That's the reason why this PR got bigger than expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor performance tweaks ...
Just building to test this.
Unfortunately the library is empty with this PR.
|
src/database/mixxxdb.cpp
Outdated
} | ||
params.filePath = QString(QStringLiteral("file://%1")).arg(filePath); | ||
params.filePath = kUriPrefix + absFilePath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want here:
params.filePath += absFilePath;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈 Fixed
Merged #2513 into this PR to prevent merge conflicts. |
Hopefully all done and fixed now. Those many open branches and PRs are hard to manage. |
Unfortunately my library is still empty after merging this branch. This is the Log output:
|
@daschuer I have absolutely no idea why this doesn't work for you. My plan is to split this PR into two separate parts: URI + everything else. |
...that allows to add extra options when opening an SQLite database connection.
Rebased the URI in-memory database commits on master. The changes turned out to be completly independent of the LibraryScanner refactoring in #2515 . Both PRs solve the testing issues in a different way, both are justified. |
I am sorry for you re extra efforts because of my DB migration issue. This one looks and works still good. |
Disable LibraryScanner during library testsFix runtime dependencies of LibraryScanner #2515Remove dependencies on TrackCollection from LibraryScannerFix runtime dependencies of LibraryScanner #2515Use Qt::DirectConnection for signal-to-signal connectionsFix runtime dependencies of LibraryScanner #2515Our connection pool does not work correctly with anonymous in-memory database connections that are used for testing. Only the database schema of the connection on the main thread will be initialized. Connections on other threads start with a fresh database and all queries are doomed to fail!
The tests succeed, but the log is spammed:
Fixed by using a named in-memory connection with the actual file name and an additional
mode
parameter.Nevertheless I have disabled theLibraryScanner
insideTrackCollectionManager
during tests to avoid issues caused by concurrent database cleanup tasks that are executed by theLibraryScanner
on startup unconditionally.TODO (see code): The#2515LibraryScanner
doesn't belong in there and should be extracted and decoupled fromTrackCollectionManager
in the long term.