Deprecate the DRT Windows-specific versions of ODBC.

[LightBender]

ODBC is supported on all major OSes and should not be limited to Windows in D. Furthermore, Database access interfaces are never appropriate in the Runtime.

Related to and supersedes: dlang/phobos#8748
Pairs with: dlang/phobos#8804

[dlang-bot]

Thanks for your pull request and interest in making D better, @LightBender! 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#15560"

[rikkimax]

sqlext.h:

// Contents: This is the include for applications using the Microsoft SQL Extensions

So no, we can't get rid of it. It needs to be part of WinAPI.

[LightBender]

@rikkimax I am working on a companion PR that puts this back in Phobos where it was before. I just need to clean up some of the references to other windows specific files that are pulled in simply for some type aliases first. There really is no need to import those files as all the types aliased can be recreated specifically for ODBC while maintaining ABI compatibility. ODBC is available on all major OSes and we need to support it on all major OSes. DConf week is my window to rectify this issue.
I'll link the PR when it's posted.

[LightBender]

@rikkimax Here is the PR that adds this back to Phobos: dlang/phobos#8804

[CyberShadow]

Sorry, I don't understand how this could be a good direction that we should go in.

Windows API bindings should stay in Druntime.

More generally, OS API bindings should be in Druntime (or maybe Deimos for the more specialized ones, but if Phobos needs them, then they should be in Druntime).

OS API bindings should definitely not be in Phobos.

[CyberShadow]

If there are parts of the Windows API that are actually suitable for all platforms, then I think it would be OK to copy them. We could also selectively remove version (Windows): from these modules, and create modules with non-Windows-specific module names that alias to or selectively import from the Windows API headers. Some other examples where this could be done is declarations of file formats, such as Windows bitmap images or PE files; if the same applies to these modules (e.g. if they contain a description of a network protocol or file format), then the same could apply.

There are some modules in the core.sys.windows package that are indeed not part of the Windows API, but are helper modules which implement some code needed for Druntime to interface with various aspects of the Windows API. However, I don't think the modules changed in this PR are ones of these; these are simply mechanical translations of the MinGW C headers. We should not move these around on a one-by-one basis; it is much more valuable for them to have the same predictable location as they are in the Windows API C header hierarchy.

In short, whatever the final goal is, I don't think this is a good way towards it.

[LightBender]

@CyberShadow Ok, there are a few points to address here.

The first appears to be an assumption that ODBC is Windows specific. It is not. ODBC is an open standard that is available on all major operating systems and ODBC drivers are provided by nearly all SQL based database vendors. This includes, but is not limited to: MariaDB, PostgreSQL, Redshift, etc. ODBC is categorically not a Windows API that is suitable to non-Windows platforms and treating it as such is how we got here. The fact that the D bindings came from MinGW is likewise wholly irrelevant to the discussion. The MinGW header translations (ODBC 3.8) are presently out of date and ODBC 4.0 reference headers have since been supplied by Microsoft here: https://github.com/microsoft/ODBC-Specification/

Second. ODBC is not an OS-level API and therefore has no business being in DRT. To my knowledge, no operating system ships with ODBC by default or as part of it's OS API surface. I have worked with ODBC on Windows and Linux and in both cases I had install a package prior to being able to access ODBC.

I suspect that the ODBC headers were initially place in etc.c.odbc by somebody who knew the above and correctly placed them in Phobos. Then later somebody else came in and realized that these are also in the MinGW translations and decided that we should just use those. The problem is that the mechanical translations of MinGW (mostly correctly) assume that MinGW is Windows only. But because ODBC is not a Windows-only feature, using the mechanical translations introduced a bug for non-Windows platforms. That an API merely originates from the MinGW translation does not guarantee that the API is Windows-only and that is a faulty assumption on the part of the maintainers that forces an incorrect version constraint on the code as an unintended consequence.

If ODBC was a) Windows-only and/or b) even an OS level API at all, then I would agree with you, but as neither of those cases are true, then Phobos is the correct location and version(Windows) is incorrectly applied.

[CyberShadow]

Thanks for the context. I agree with everything you said except:

then Phobos is the correct location

The correct location is the current location for the reasons that I've already explained, as the provenance of the files is still the most important aspect for dictating their location. Moving things around will complicate updating to a newer version of the MinGW headers.

So, yes, feel free to selectively remove version (Windows): from select headers as needed.

If at a certain point the high-level ODBC wrappers in Phobos will need things that are missing or inadequately specified in the MinGW headers, then a separate translation can be added (also to Druntime, IMO) from a better source.

[CyberShadow]

As for the reason why Druntime and not Phobos... whether they are OS bindings or not is not relevant I think. Druntime is for things which are not opinionated, and that any standard library may use. So hypothetically if Tango still existed today and wanted to provide an opinionated high-level ODBC wrapper using its own approach, it would still be able to benefit from the same bindings as Druntime, because there isn't really more than one way to wrap a C API in D. It's also the motivation for having a centralized project for other bindings, Deimos.

[LightBender]

So, yes, feel free to selectively remove version (Windows): from select headers as needed.

And every time the translations are updated we have to remember to remove the version specifier. No matter what, you're introducing a special case, either we special case removing the ODBC headers from translation, or we special case removing the version specifier from the translated files. Both cases result in a manual "checklist" item that has to be checked on a process that is run infrequently at best.

The correct location is the current location for the reasons that I've already explained. Moving things around will complicate updating to a newer version of the MinGW headers.

This gets to a second problem that I suggested. The MinGW headers (or at least the translations) are out-of-date. ODBC 4.0 has been around since 2016, and yet the current version that is in the translations in ODBC 3.8. In fact this is a major problem with using MinGW in general. I have encountered multiple situations where even Windows APIs were not available in the MinGW translations. Part of the reason I am doing this work right now is that my next PR is upgrading the headers to ODBC 4.0, which is not apparently available in MinGW.

Realistically, the correct long-term answer for Windows APIs in using ImportC on the Microsoft-provided Windows headers as the MSFT headers are both always current as well as being canonical. But interestingly, if we did that, ODBC would disappear because ODBC is not part of the Windows API surface. This, in my opinion is a strong argument against keeping the APIs in the DRT Windows API. ODBC's present location in core.sys.windows is an artifact of the MinGW translations, and not because they actually belong there.

Druntime is for things which are not opinionated, and that any standard library may use.

That's an interesting theory for DRT usage, but to my knowledge, there is nowhere to actually put non-OS specific library interfaces in the current module structure, which suggests that DRT is not presently intended or designed to be used that way. And creating such would result in moving these files to new folders, which would trigger the special casing problem for MinGW translation that we've already discussed above.

Phobos also has an etc.* section for library interfaces, in this case these are interfaces for libraries that Phobos uses, one of which is sqlite3. And this makes sense because database access is always a Standard Library feature, it's never a Runtime feature. Were D to gain a standardized SQL access implementation like ADO.NET or Java's ResultSet implementation (on which DDBC is based), it would live in Phobos, never in DRT. And thus, because any standard SQL access implementation would be in Phobos, the ODBC library interface would also live there per our present structure.

If you want to make the argument that the entire etc.* space in Phobos should be in DRT, I would be amenable to that and would amend my position, but right now, the most logical place for ODBC is in Phobos with the rest of the SQL interface libraries. And in any case, we'd still be moving the ODBC files out from their default location and special casing it during translation. Additionally, whether somebody has to import the bindings from Phobos or DRT is irrelevant because even they don't use Phobos, importing library interfaces does not mean they are pulling in Phobos itself.

[thewilsonator]

So ro recap:

• ODBC version is out of date
• OBDC is based off of MinGW headers
• MinGW headers are (translated as?) Windows only.
• ODBC is not a Windows only thing, and will get reset as windows only by any reconversion from minGW headers
• this module should be moved somewhere, etc.c or somewhere else in druntime

Decisions to be made: where should this be moved to?
it is already in etc.c albeit scheduled to be removed due to expiring deprecation (in favour of the module this PR seeks to deprecate). It shouldn't remain in core.windows as it is neither specific to windows nor an OS specific API.

[thewilsonator]

(also @CyberShadow i note that ae is broken on BuildKite, complaining that deprecated modules are deprecated)

[CyberShadow]

And every time the translations are updated we have to remember to remove the version specifier. No matter what, you're introducing a special case, either we special case removing the ODBC headers from translation, or we special case removing the version specifier from the translated files. Both cases result in a manual "checklist" item that has to be checked on a process that is run infrequently at best.

Well, ideally, we wouldn't have version(Windows): at all. The reason we have it is limitations of linkers and our own problems with ModuleInfo causing bloat for non-Windows platforms.

I vehemently disagree that adding/removing a version(Windows): line is anywhere close to being equivalent to moving the files to an entirely different Git repository!

But interestingly, if we did that, ODBC would disappear because ODBC is not part of the Windows API surface.

But the Windows SDK includes sql.h etc.?

ODBC's present location in core.sys.windows is an artifact of the MinGW translations,

I think most modules (the ones coming from MinGW) probably should have mingw in the name (and then have a windows package which publicly imports them). That way, if there was ever a need to use a header file that did not come to MinGW, it could be added without causing a mess in terms of what came from where and how one would update it.

there is nowhere to actually put non-OS specific library interfaces in the current module structure

The etc package exists in Druntime too. We have not been paying much attention to having a consistent structure, but putting them in etc. or etc.c. would be fine, regardless of whether it's in Phobos or Druntime. As to whether it should be in Phobos or Druntime, we have also not been paying a lot of attention to that, because the goal of allowing multiple "standard" libraries was never fully realized. Anyway, that's a separate and relatively unimportant point compared to the argument at hand (moving MinGW translations out of the core.sys.windows package).

And this makes sense because database access is always a Standard Library feature, it's never a Runtime feature.

OK, this is a fair argument.

If you want to make the argument that the entire etc.* space in Phobos should be in DRT

I think that would also be fine, depending on how one would be inclined to define what Druntime is, and what would personally make the most sense to me (ignoring the cost of actually moving these).

the most logical place for ODBC is in Phobos with the rest of the SQL interface libraries.

I think it would be perfectly fine for there to be modern ODBC bindings (based on Microsoft's GitHub repo) in Phobos (or Druntime) under the etc. package, alongside from the MinGW translations in the core.sys.windows package. These originate from different sources and the process of keeping them up to date is different. I think that, unless special attention was paid to maintain compatibility of the D bindings, a from-scratch translation of any C bindings will almost certainly introduce breaking changes compared to a different translation of an older version of the same bindings, so having the new ones in a new, separate namespace would then be a positive. In this case, it would be sufficient to discourage (in the documentation) the use of the older MinGW headers, instead of deprecating and outright removing them.

Additionally, whether somebody has to import the bindings from Phobos or DRT is irrelevant because even they don't use Phobos, importing library interfaces does not mean they are pulling in Phobos itself.

If only that would be true in practice... unfortunately, we are very, very far from "pay for what you use" in D.

Decisions to be made: where should this be moved to?

I say, go ahead and add the new bindings in Phobos (under etc.c); deprecating the MinGW ones doesn't immediately solve anything and I think it should not block anything.

also @CyberShadow i note that ae is broken on BuildKite

Ah, thanks for the ping, I'll have a look! (Looks like a segfault in a unit test...)

Edit: It's https://issues.dlang.org/show_bug.cgi?id=24050.

[thewilsonator]

Thanks for the update, will let Walter know tomorrow.

[LightBender]

I think it would be perfectly fine for there to be modern ODBC bindings (based on Microsoft's GitHub repo) in Phobos (or Druntime) under the etc. package, alongside from the MinGW translations in the core.sys.windows package. These originate from different sources and the process of keeping them up to date is different. I think that, unless special attention was paid to maintain compatibility of the D bindings, a from-scratch translation of any C bindings will almost certainly introduce breaking changes compared to a different translation of an older version of the same bindings, so having the new ones in a new, separate namespace would then be a positive. In this case, it would be sufficient to discourage (in the documentation) the use of the older MinGW headers, instead of deprecating and outright removing them.

@CyberShadow This is a reasonable compromise and it makes practical sense. I've removed the deprecation labels and modified the documentation notes to reflect this.

I actually I have an ImportC based version of the ODBC 4.0 bindings ready to go for Phobos, except for this bug in ImportC: https://issues.dlang.org/show_bug.cgi?id=24121

Thanks! (I guess the Phobos addition is still a prerequisite to this PR, then.)

[thewilsonator]

Phobos is now in.

[ibuclaw]

OS API bindings should definitely not be in Phobos.

+100

The only good C bindings in Phobos are those behind package(std).