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

Consistent #include's in public VM APIs #35172

Closed
mehmetf opened this issue Nov 14, 2018 · 3 comments

Comments

Projects
None yet
4 participants
@mehmetf
Copy link

commented Nov 14, 2018

Currently, the #include's in runtime/include/*.h follow two formats:

#include "dart_api.h"
#include "dart_tools_api.h"

and

#include "include/dart_tools_api.h"

This creates problems when we try to define the includes root. Currently, we have to put in both /runtime/ and /runtime/include as part of -I which seems unnecessary. Can we please standardize? I would much rather we always do #1 (w/o the "include" prefix).

@keertip

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

@a-siva , this will make it easier to build internally.

@a-siva

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

Can you elaborate on why you prefer the first option.
In the runtime/vm directory we always use #include "include/dart_api.h" this has been our stated policy and i see that some of the newer files that got added in the 'include' directory do not follow this.

@mehmetf

This comment has been minimized.

Copy link
Author

commented Nov 15, 2018

We currently recommend users of this API to write #include "dart_api.h" in their code as opposed to "include/dart_api.h". The latter makes less sense to me since the prefix include/ is not specific to Dart VM.

These users, internally, depend on a single build target which defines the -I flag so they can't modify it. So we either need to tell them to use the include prefix OR have both runtime/ and runtime/include/ as part of -I.

@a-siva a-siva assigned liamappelbe and unassigned a-siva Nov 15, 2018

dart-bot pushed a commit that referenced this issue Nov 15, 2018

Standardize the #includes in runtime/include/*h.
Bug: #35172
Change-Id: I75d1e99fb178d7b6df051f6ada0a26a126e31b81
Reviewed-on: https://dart-review.googlesource.com/c/84542
Commit-Queue: Liam Appelbe <liama@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>

@liamappelbe liamappelbe reopened this Nov 16, 2018

dart-bot pushed a commit that referenced this issue Nov 19, 2018

Rollforward of the dart_io_api.h include change, with a fix for flutter.
The fix for the flutter build was to switch include_dirs from . to include/ in the dart_public_config. The other files in include/ were compiling correctly because their #includes were being treated as relative paths, but dart_io_api.h is in a subdirectory. In future, clients should be able to just add include/ to their C++ includes.

Tested: Dart tests, and verified that flutter engine compiles.
Bug: #35172
Change-Id: Ic74d7c1cdedeac32df939c53c1fd57a1260072a3
Reviewed-on: https://dart-review.googlesource.com/c/84686
Reviewed-by: Siva Annamalai <asiva@google.com>
Commit-Queue: Liam Appelbe <liama@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.