Conversation
|
@wpferguson - Sorry for not commenting on your RFC in time, but I believe we should NOT make building with Lua mandatory. Integrating Lua into darktable code can cause issues and it is vital to be able to build darktable without Lua for debugging purposes. |
|
Right, IIRC this could cause issues for GNU/Linux packagers. |
|
If a feature is enabled by default, then the inability to build with that feature due to missing prerequisites should result in a build failure, rather than silently disabling the feature and building successfully without it. |
|
@victoryforce I replied to your PR before reading this.
I don't know enough about packaging to understand. The only issue I see would be shipping 2 versions of Lua though I'm not sure why that would be a problem. I have 5.1, 5.2, 5.3, and 5.4 on my system with no issues. The packagers could make 5.5 the default and include 5.4 for compatibility reasons.
Could you give me an example? @TurboGit I think my solution is the correct one 😄 , but I can live with @victoryforce's solution. So, you choose. |
I don't really understand why this needs to be proven with examples. Lua is very deeply integrated into many critical parts of the darktable code. If your proposed change is to make it impossible to throw this code out for debugging purposes, then you should prove that Lua integration cannot cause issues. In any case, doesn't the quote from the comment in your last PR serve as an example? "If the current view is set to lighttable, but the view hasn't completely initialized, and Lua attempts to install the lib the result will be a hang." (emphasis mine, of course)
I'm not disputing that your solution is correct, of course it is correct in the sense that it solves the problem of a regression unnoticed by the package maintainer when rebuilding the package. I'm trying to show that it is not optimal considering other factors. |
That's not a Lua problem, it's a view/views.c problem. Lua just makes it easier to reproduce the issue. My concern is that if it's a feature, then it's considered optional and I think we're past that point. |
I'm not trying to find the culprit for the problem and say it's Lua. This is just an example of how supporting Lua creates additional execution paths. The bug could be somewhere else, of course, but having Lua present could expose it and lead to bad consequences. Again, this isn't about the culprit. This is about the execution paths.
Distributions don't try to package apps with disabled features, it just doesn't make sense. That's not their approach. |
Lua is used to fix issues in master, but is currently an optional feature. In order the issue fixes to be applied Lua must be present, so it needs to be a required package.
Fixes #20683