-
-
Notifications
You must be signed in to change notification settings - Fork 230
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
Mutually exclude mainSourceFile of different configurations #2039
Conversation
Thanks for your pull request and interest in making D better, @omerfirmak! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. |
4b6d451
to
eaffba6
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.
Nice! This needs a release note entry. Suggestion:
Dub will now automatically exclude
mainSourceFile
from other configurationsBy default, Dub uses all files it can find under its
sourcePaths
.
However, a common pattern when dealing with multiple targets is to use
configurations to represent said targets. In the case those targets are executables,
users would be forced to add main files from other configurations to the
excludedSourceFiles
list, or store the main in a different directory outside of
thesourcePaths
.To simplify this workflow, Dub will now exclude files listed in
mainSourceFile
for other configuration. In case this is not desirable, the files need to be manually
added to thesourceFiles
list.
Side note: When writing release notes, we always try to include the following 3 things:
- Previous situation;
- Reasoning for the change;
- Mitigation process (in case the user actually wants the old behavior);
2ada756
to
976d6f1
Compare
Just in case: https://github.com/dlang/dub/tree/master/changelog |
Unless explicitly added to sourceFiles, a configuration will exclude other configurations' mainSourceFile's.
976d6f1
to
0a20856
Compare
Thanks! Added your suggestion to changelog. |
A configuration's settings should not be affected by other configurations. This has broken mysql-native. I'm not sure why (will ask the original maintainer), but it uses the package file in one configuration as the mainSourceFile. With this change, how does one write a dub recipe file that works across multiple compilers? I'm assuming that "mainSourceFile" was there for a reason. I get the idea behind the change, and it's a good idea. However, I think we can do this without breaking existing projects. Maybe introduce an "exclusiveMainSourceFile" or something? |
Another artifact here is that dub is spitting out these exclusions into the local package cache, so even using a previous version of dub will not work once the cache has been set up by the 2.095 version. |
First, sorry for breaking your library. We weren't aware of that. I'd recommend adding
While I usually agree, I think this is a (useful) concession to a common use-case, and the work-around for this is easy.
I'm not sure I follow why this change would make writing cross-compiler recipes harder ? Looking at your recipe, you just have to remove the offending line to make things work again. From what I can see of https://github.com/mysql-d/mysql-native/blob/27ea51f8b0c6aa41aab00fc288a2a1dec734905f/dub.sdl, half of the file is just working around dub limitations (such as wanting optional dependencies to actually work). And given that
We had another change that was halted because it broke projects. Unfortunately it's pretty hard to know unless we either 1) do nothing or 2) have a comprehensive test-suite, hence my suggestion to add mysql-native to the project tester. IMO we should really come up with a list of needed changes for the near future and implement them in DUB 2.0. Because I really don't want to break anyone's workflow out there, but at the same time, no one is happy with their DUB workflow. |
Not what I meant. What I meant was, if, for instance, I want to write a dub file that works with both dmd 2.095 and 2.094, dub has subtly changed the meaning of this directive between those 2 compilers. Changing the meaning (significantly) in one compiler release without deprecations or warnings is not good practice. Granted, dub can be installed separately from the compiler, but it's usually packaged in all of the installers.
I'd also recommend getting rid of that warning. The original maintainer said he can't really remember why he added it, but he thinks he added it to be consistent with rdmd, which requires you to name a "root" file from which all other things are imported. But that warning probably had something to do with it.
I think this change has to be reverted. Changing the meaning breaks the intent of previous recipe files (even if that intent was misplaced). I shouldn't have to add a project to some CI tester to ensure the language doesn't change from under my feet, that's not a way to develop a widely used tool. At least a deprecation cycle is in order. I think the original directive was poorly defined to begin with, and should be deprecated in favor of something more spelled-out. Thinking about it, it really should be "excludeInOtherConfigs", which conveys the actual effect, rather than some fuzzy "this is where the main file lives" to be acted on (poorly) by dub test (the goal of which, by the way, is moot now that unittests do not run main by default). Oh, and this should NOT be done the way it was in any case -- injecting |
I just tried the new dub version and this change breaks all of my repositories I wrote at work. I am used to the pattern to have in Due to this pr changes, it fails now with linker errors. The solution seems to be to layout the test main to a separate It is even more worse. I am somehow experienced with dub and therefore know why there are linker failures. But for less dub experienced user they have no chance to find out why linker errors occurs. |
@schveiguy @andre2007 I discussed with @Geod24 and we think the best course of action is to revert this change, but more specifically to revert to the old behavior and make the new one opt-in. What do you guys think? |
Yes, I agree |
Actually, the main reason for the existence of this directive is to support rdmd based builds, |
BTW, @Geod24, I don't agree with the push for 2.0:
I hope we are on the same page that you will really only ever see the complaints, but not the cases of people simply being happy. I have no idea how the actual percentages are for the various levels of problem severity, but I certainly know that this kind of black/white painting doesn't help overall. Personally, of course I also have some places with workarounds or solutions that could be more nicely integrated, but overall I'm pretty happy and I know that non-breaking additions, such as extension support or parallel builds, will get rid of at least my remaining issues. |
Well, you're just describing breaking changes here. This has nothing to do with compiler version.
Yep, I wanted to look into this.
I agree.
That is not at all what I was suggesting. I suggested that, as a mature and widely used project,
In case that wasn't clear, the breaking change was not anticipated (and yes, in hindsight, it should have been, but hindsight is 20/20.
I'm going to strongly disagree here. We have already too many ways to do things, and adding an extra field for a slightly-different behavior is just bad design. The core issue that this PR was trying to solve was that it's still convoluted to have multiple projects in the same source tree.
That's a good point, thanks for the feedback. |
You're making an argument against any change in general here. We're stuck in a situation where we have an application, which is also a library, that is extremely resistant to change as it must not break compatibility. We end up with things being piled on top of one another with no coherence. We have two package formats, configurations vs sub-packages (and to some extent, build modes), a few ways to override package providers (such as file system providers vs What I'm suggesting is defining a supported set of features, for a supported period, and every once in a while (e.g. 2 years), simply revise things and improve the status quo.
I'd be happy if you provide a list. Because the improvement that mattered to us are:
We also had #2035 which is also a change of behavior, but we got this bug reported so many times that I don't think anyone would dare to argue against it.
We are on the same page. I'm sorry if you felt I was being too negative towards dub. It's no easy feat to maintain a language's package manager for as long as you have been, and I don't think you get enough credit for. However, I do think you have a workflow that suits the tool you built. Not everyone does (I know my workflow for instance is pretty unique), and I think by polling the combination of people's workflow, we could end up with a tool that is much more satisfying to use than it is today. But to integrate the feedback we got, we'll need to make some changes which will break users' recipe in subtle ways. In the case of this specific PR, there is a backward compatible change one can do to their recipe to make it work both with the old and new dub. And as mentioned, both use cases are here to work around previous limitations. |
I agree. But what does this mean? A command line switch? A new directive? I prefer a new directive. |
Thanks for this bit of history, it makes things clearer to me. The mysql-native maintainer before me said this was probably due to using rdmd in some cases. But still, the idea that |
dub ships with the compiler. In fact, on my mac, I use dvm to manage compilers, and it uses the dub that installs with that compiler. True, it's possible to use dub as a separate installation. Not many do that though. |
What I meant is that making any sorts of backwards compatibility "cuts" in the form of a major version has the risk of people locking in to the old version, because it is more convenient or the only possibility (external dependencies). Shipping with the compiler helps of course, but also makes the problem worse when having to support multiple compiler versions. Also, "breaking change" is something that in the end needs to be defined in terms of practical risk (probability and severity) with some threshold, otherwise we would be at version 25.0.0 by now. At some point there will be a 2.0.0, no doubt, but even then I'd strongly prioritize backwards compatibility w.r.t. the package recipe. That may mean that some compatibility code needs to be kept around across major versions, but packages that declare a minimum dub version for example would run with only the new semantics.
Apart from what's there, what possible features on the radar do you think would be incoherent with the current feature set?
If that means breaking existing features every two years then I simply don't think this is a good idea. With appropriate deprecation periods and a compelling justification for removing a feature, this is fine, but the trade-off needs to result in a net positive for the ecosystem as a whole. Just breaking or removing little things is hardly solving the real issues. And certainly not removing core features such as configurations or sub packages.
Commented on the issue. This is obviously not critical, but makes sense to be fixed long term.
The way this is supposed to work in the usual cases is that DUB knows which flags need to or may be propagated and acts accordingly. This is currently works for build options (
I think overall this still lies in the non-breaking spectrum.
This is definitely a breaking change, especially since this is officially documented and probably should be reverted. The overlap of input and output variables is indeed an issue, though, so a transition starting with the documentation and a deprecation message when used directly in a build commands directive is definitely desirable.
Agreed, the risk of this breaking anything seems to be low and it definitely was a stepping stone.
I agree with all of this, except for "But to integrate the feedback we got, we'll need to make some changes which will break users' recipe in subtle ways" as a generalization. Of course any change that is visible from the outside has the potential to result in subtle or obvious breakage, so every change will always be a judgment call in that regard. But even if the direct change is deemed breaking, usually there is an acceptable transition to the desired behavior and I think most open changes that have a high impact are well outside of the breaking territory anyway. Just for this particular issue it is obvious that there is a real chance that this will mess with existing code, it's just a convenience feature, and I agree with Steve that configuration directives should not interfere with each other, as it adds an unnecessary element of surprise. |
I think with Druntime now supporting to skip the execution of |
So is there a plan to revert this and reintroduce via something else? Just asking, because mysql-native is broken with the latest dub until this is resolved. |
Just starting to get into cleaning out the mysql-d cobwebs. Has this change been reverted or fixed? |
Users still having issues due to this change https://forum.dlang.org/post/menbaobefkvwrychtgar@forum.dlang.org |
Unless explicitly added to sourceFiles, a configuration will exclude
other configurations' mainSourceFile's.