Skip to content
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

Add external import path switch #20899

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add external import path switch #20899

wants to merge 1 commit into from

Conversation

rikkimax
Copy link
Contributor

Before I attempt to add the switch itself, let's make sure I didn't break anything with my refactoring of the DllImport detection logic.

Based upon this logic, I can see that the -dllimport=externalOnly option wouldn't be required.

I cannot foresee a reason why you would want to set the explicitly external flag on a module, without also wanting it to be DllImport.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @rikkimax! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

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 references

Your 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 locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#20899"

@rikkimax
Copy link
Contributor Author

The switch itself needs a test, (although I have yet to figure out how to do it).

What is here, is already covered by the testsuite.

@thewilsonator thewilsonator added Review:Needs Changelog A changelog entry needs to be added to /changelog Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org labels Feb 19, 2025
@rikkimax
Copy link
Contributor Author

Why does this need a spec PR?

This is for CLI switch, and that gets updated automatically based upon the table in the compiler.

@thewilsonator
Copy link
Contributor

Why does this need a spec PR?

Documentation

@rikkimax
Copy link
Contributor Author

Which is automatically generated from the CLI switch table in the compiler.

So what are you expecting in the spec to change?

@thewilsonator thewilsonator removed the Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Feb 19, 2025
@rikkimax rikkimax force-pushed the extI branch 2 times, most recently from 9b39c1e to 350582a Compare February 19, 2025 12:09
@rikkimax
Copy link
Contributor Author

Upon reflection, the existing DLL tests cannot be made to work.

They require both druntime and phobos to exist as DLL's which are not current produced.

I either have to implement a -betterC test for this from scratch, or put it in without a test.

@rikkimax
Copy link
Contributor Author

Upon review, the dllimport override switch would be required, as none would quit the process of determining DllImport status too early.

I have also determined that cxx_dll test should work for the test case. As long as its just importing a single global variable.

@rikkimax rikkimax force-pushed the extI branch 3 times, most recently from 2ac3bcd to de96a44 Compare February 19, 2025 13:25
@rikkimax
Copy link
Contributor Author

I'm expecting on Windows the dll_cxx test to fail, I didn't add the -dllimport=externalOnly to its args.

@rikkimax
Copy link
Contributor Author

rikkimax commented Feb 19, 2025

There we go! Error: undefined reference to `_testExternalImportVar`

We have confirmation that without -dllimport=externalOnly it fails, now to confirm that it and -extI is working.

@rikkimax
Copy link
Contributor Author

I don't understand why Azure pipelines (Windows_DMD_bootstrap x64) is failing. It's not dll_cxx test.

I'll need to review what I changed in the checks to make sure I didn't mess that up.

@dkorpel
Copy link
Contributor

dkorpel commented Feb 19, 2025

Azure randomly fails sometimes

@rikkimax
Copy link
Contributor Author

Azure randomly fails sometimes

Indeed, after review, I think that is the case here. It was a DLL test, which is why I got suspicious.

@rikkimax
Copy link
Contributor Author

Okay it's now green.

The tags can be removed, there is a test dll_cxx and a changelog.

@ibuclaw for GDC I am assuming you'll need to name it something like --d-extI, do whatever you need to.

@kinke I'd appreciate a look, from what I see for ldc the dllimport detection logic is in two functions so its going to need a bit of work. If we need to discuss who does what for ldc we can later, there is no point doing it here until after a downstream.

@rainers you wrote the code, so if you want to check it over, here is a ping.

@rikkimax rikkimax marked this pull request as ready for review February 20, 2025 03:28
@rikkimax rikkimax requested a review from ibuclaw as a code owner February 20, 2025 03:28
@thewilsonator thewilsonator removed the Review:Needs Changelog A changelog entry needs to be added to /changelog label Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants