dlang / dub

Package and build management system for D
MIT License
678 stars 227 forks source link

Add collection of c sources and headers to dub #2544

Closed cschlote closed 1 year ago

cschlote commented 1 year ago

Add changes to support C source and header files with Dub

This PR adds support for collecting C source and header files. The goal is to be able to use C files (.c, .h and .i) in Dub projects the same way as usual D files (.d and .di). Dub shall be able to pick up these files automatically and pass them to the D compiler.

To maintaining backward compatibility, new attributes were added to JSON/SDL files: 'cSourcePaths' and 'cImportPaths'. If specified, dub will pick up C sources found in the specified paths, and additional import paths are passed the compiler.

Note: Currently there is no way for the existing D compilers to specify separate D import paths and C header paths, so the paths are currently joint and passed as import paths. This however might change, if the D should be changed to support separate arguments.

The PR replaces PR #2522. The patch stack is squashed and all unreleated changes are removed.

Geod24 commented 1 year ago

Just going off the description here, but:

To maintaining backward compatibility, new attributes were added to JSON/SDL files: 'cSourcePaths' and 'cImportPaths'. If specified, dub will pick up C sources found in the specified paths, and additional import paths are passed the compiler.

I am quite opinionated against this.

cschlote commented 1 year ago

@WebFreak001 argued well for introducing and using such new attributes. Simply to avoid, that dub suddenly starts to pickup and pass C sources to the D compiler. He mentioned, that some people - like Walter Bright - keep C sources in the D source directories while porting them to D. One of my older projects broke for the same reason. Suddently dub started to pickup C sources and passed them to the D compiler while some preBuild script did manual compiling of the C sources. This doesn't work out.

For this reason the new 'attributes' for SDL/JSON were added. You must explicitly specify the directories, where the C files are searched and picked up. Without these new attributes the behaviour is exactly as before.

WebFreak001 commented 1 year ago

I am strongly for making them separate properties, like they are done in this PR right now. There are many advantages over merging them into existing source paths:

I think for ease of use we could also consider adding a "processC": true property, which could simply append sourcePaths/importPaths to cSourcePaths/cImportPaths. I would be open to include that in this or in a separate PR.

I don't think there is much value in enabling processing C source code in existing projects without the user knowing it. For new projects we could think of some convention like "The c/source/ folder is used as importC path by default" in the future. Having separate values first allows us to think about the conventions in more detail and see how users are adopting importC into their projects.

WebFreak001 commented 1 year ago

fixes #2269

@Geod24 do you have more comments for this? Should we go ahead with this or do you think a different approach would be better? See also my last post.

cschlote commented 1 year ago

Hi. Any news on this? Anything to change? Should I rebase to latest master?

I would really like to see some better support for ImportC in dub ...

WebFreak001 commented 1 year ago

I would be for merging this if @Geod24 doesn't have any more comments.

You should add a toolchain requirement file to the test folder, so the auto-tester doesn't try to run it with old compilers.

Also rebase / merge master, then we can move forward I think.

cschlote commented 1 year ago

You should add a toolchain requirement file to the test folder, so the auto-tester doesn't try to run it with old compilers.

There is a "toolchainRequirements" field in test/use-c-sources/dub.json. The entry for 'dub' itself must updated to the tagged 'dub' release following the merge.

Or do you mean some other file?

WebFreak001 commented 1 year ago

I meant this kind of min_frontend file, which I think prevents the test from being done by the test runner in the first place.

But we will see if the tests pass now

Example file: https://github.com/dlang/dub/blob/master/test/issue1427-betterC/.min_frontend

WebFreak001 commented 1 year ago

dmd master breakage just got in, unrelated to this PR. For alpine CI steps it looks like it doesn't recognize the frontend version properly.