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 collection of c sources and headers to dub #2544

Merged

Conversation

cschlote
Copy link
Contributor

@cschlote cschlote commented Dec 3, 2022

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.

@cschlote cschlote force-pushed the add-collection-of-c-sources-and-headers branch from 7d0a1c9 to 1339484 Compare December 3, 2022 14:23
@Geod24
Copy link
Member

Geod24 commented Dec 3, 2022

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
Copy link
Contributor Author

cschlote commented Dec 3, 2022

@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
Copy link
Member

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:

  • we ensure no old projects with C source code within the source folder break
    • this is especially important for people who auto-convert between C and D and may share files (in both directions)
    • random linker errors could otherwise appear when the compiler starts building .c files into the executable
    • C binding libraries could suddenly start to compile differently and taint imports
  • third party tools (e.g. serve-d or other IDEs) can turn on C/C++ features of the IDE on-demand and potentially integrate with it explicitly. (and show the user warnings if no C/C++ support is installed, but C is attempted to be integrated)
  • with this we keep future decisions on how to proceed with defaults and conventions still open (we can still default enable it in the future if it turns out useful)

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.

@cschlote cschlote force-pushed the add-collection-of-c-sources-and-headers branch from 1339484 to 2a5ede3 Compare December 13, 2022 07:10
@cschlote cschlote force-pushed the add-collection-of-c-sources-and-headers branch from 2a5ede3 to 77af108 Compare January 9, 2023 18:56
@cschlote cschlote force-pushed the add-collection-of-c-sources-and-headers branch from 77af108 to 1e13eaf Compare February 3, 2023 18:09
@WebFreak001
Copy link
Member

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
Copy link
Contributor Author

cschlote commented Feb 25, 2023

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
Copy link
Member

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 cschlote force-pushed the add-collection-of-c-sources-and-headers branch from 1e13eaf to 558191c Compare February 26, 2023 11:50
@cschlote
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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.

@WebFreak001 WebFreak001 force-pushed the add-collection-of-c-sources-and-headers branch from 1b9dcbe to f98cbf0 Compare March 1, 2023 20:38
@WebFreak001 WebFreak001 enabled auto-merge (rebase) March 1, 2023 23:29
@WebFreak001 WebFreak001 merged commit a7b92b8 into dlang:master Mar 1, 2023
@cschlote cschlote deleted the add-collection-of-c-sources-and-headers branch March 2, 2023 15:44
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.

3 participants