-
Notifications
You must be signed in to change notification settings - Fork 29
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
Improve mime type deployment #135
base: master
Are you sure you want to change the base?
Conversation
ee8acf2
to
40813ce
Compare
40813ce
to
6b1fed2
Compare
77256c1
to
40a267d
Compare
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.
Who would ever think downloading unstable possibly broken tools is a good idea for testing? Tests are documentation, they must not rely on external resources. Rather they must be completely self-containing.
Big no-go.
message(STATUS "Generating test data AppImage") | ||
|
||
# Get appimagetool | ||
set(APPIMAGETOOL_BIN ${CMAKE_CURRENT_BINARY_DIR}/appimagetool) |
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.
This change is completely unrelated to the actual functionality you added. I am not a fan of this at all. It's a horrible concept. You should not rely on unstable moving targets in unit tests! NEVER!
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.
This change is required to avoid adding another AppImage binary to the repository. And it totally related to the change being introduced as it provides the test data for the test cases that validates the correct behavior of the code.
I understand your concern about depending on unstable releases so the latest stable release is being used instead.
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.
These external dependencies you mention however are all pinned in some way. You introduce a dependency on an unstable system. The case where such a dependency might be broken and tests start failing at random is unacceptable. The reason is that unit tests should not just fail randomly, that makes debugging extremely hard. Unit tests should be as self-contained as possible, and even if there's external dependencies, these must be pinned so you can at any time restore a given point in the development history. Otherwise, things like reproducibility of bugs aren't provided any more. That's really a no-go.
If you find a way to pin that stuff somehow, your change is more viable.
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.
It's already done, now the code only downloads the 12th release of appimagetool so a random broken build will not break the tests cases.
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.
If you open an issue to change this to something less download-stuff-from-Internet, then this might be viable. The problem is that this again breaks builds on e.g., OBS.
CC @probonopd.
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.
That also doesn't solve our principal problem, which is that we have no means of generating a complete list of all dependencies, and check whether we're complying with all of those. Not at all. It might contribute, but it also has lots of drawbacks, e.g., for OBS.
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.
It does resolve this generating a complete list of all dependencies. About OBS, I have to google a bit :)
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.
What a naive claim. As said, it might contribute, but:
a) we still have to check all of our own packages for conan for compliancy
b) as soon as you use anything third party, you need to look into those by hand to check whether all licenses have been declared correctly
c) there will always be non-conan dependencies
Don't be so naive to think license compliance was that easy. That list will not be complete, though it might come very close.
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.
Well, if it contributes to the solution it's not a naive idea. As you know I already use conan as part of AppImageService and is working really well.
a) I already have done that part, had to apply some small patches but nothing outstanding. Final apps like AIL doesn't need to be turned into conan packages.
b) Licenses are included in conan packages, so it's not complicated at all to check it. I wonder if there is a package manager that checks licensing issues.
c) Yes, the linux kernel and compilers. Besides those is possible to have a conan package for almost everithing. There are conan packages even for glib.
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.
Sadly OBS is not among the conan officially supported CI platforms, details in https://docs.conan.io/en/latest/integrations/ci.html
We already depend on a lot of external projects to build |
fix typo Co-Authored-By: TheAssassin <[email protected]>
fix typo Co-Authored-By: TheAssassin <[email protected]>
fix typo Co-Authored-By: TheAssassin <[email protected]>
use namespace alias Co-Authored-By: TheAssassin <[email protected]>
use namespace alias Co-Authored-By: TheAssassin <[email protected]>
Deploy Mime Type icons and edit Mime Info Packages to properly point to the deployed icons.
Fix for #23
Notice that this PR uses in the unit tests an AppImage that is generated on configure time. This is our first step towards #116