-
-
Notifications
You must be signed in to change notification settings - Fork 652
makedep: add missing libs from pragma(lib) #21390
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request and interest in making D better, @xoxorwr! 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. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#21390" |
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.
Could you add a test ?
compiler/src/dmd/root/string.d
Outdated
/** | ||
* Checks if a given path points to file with an extension. | ||
*/ | ||
bool hasExtension(char[] path) |
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.
You could just use dmd.root.filename : FileName.ext
: FileName.ext(path).length
.
compiler/src/dmd/pragmasem.d
Outdated
@@ -247,6 +249,12 @@ void pragmaDeclSemantic(PragmaDeclaration pd, Scope* sc) | |||
ob.writestring(name); | |||
ob.writenl(); | |||
} | |||
|
|||
if (global.params.makeDeps.doOutput && name.hasExtension) |
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.
hasExtension
looks like an arbitrary condition here. The commit message says "Exclude system libs (without ext)" but I don't think that's how it works. kernel32 is a system library on Windows, but I link it with: pragma(lib, "kernel32.lib")
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.
Oh you are right, I guess there is no reason to exclude anything?
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 the thing is libraries are resolved by searching them through a list of library paths, the first of which is the current directory. If it is inside there, it makes sense to add it as a make dependency, but if it's not, then make won't be able to find it so it won't work I think (though I haven't tested this yet).
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.
Oh right, this is another problem - resolving the ref to an actual path. I don't think that can really work reliably.
I don't think this is correct - it's only for linking that a changed external library makes a final binary dirty. When compiling only, a changed library doesn't make the produced object files / static libraries dirty. |
Build tools like xmake depend on that file to be able to implement incremental compilation, it was missing static libraries from
pragma(lib)
so it resulting inundefined reference
errors