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.

Sign up for GitHub

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

__declspec(dllimport) added to headers for windows #180

Merged
merged 4 commits into from
Sep 18, 2023
Merged

__declspec(dllimport) added to headers for windows #180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d7e9ee
test for constants added, ci test run added
milyin Sep 13, 2023
7fbbf50
__declspec(dllimport) added
milyin Sep 13, 2023
a87f0a6
disabled failing alignment test
milyin Sep 13, 2023
d024897
constants test corrected
milyin Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,22 @@ jobs:
cmake .. -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=~/local
cmake --build . --target install --config Release

- name: run cmake tests with zenoh-c as dynamic library
shell: bash
run: |
cd build
cmake .. -DZENOHC_BUILD_TESTS_WITH_STATIC_LIB=FALSE -DCMAKE_BUILD_TYPE=Release
cmake --build . --target tests --config Release
ctest -C Release --output-on-failure -E z_api_alignment_test

- name: run cmake tests with zenoh-c as static library
shell: bash
run: |
cd build
cmake .. -DZENOHC_BUILD_TESTS_WITH_STATIC_LIB=TRUE -DCMAKE_BUILD_TYPE=Release
cmake --build . --target tests --config Release
ctest -C Release --output-on-failure -E z_api_alignment_test

- name: make examples with zenoh-c
shell: bash
run: |
Expand All @@ -64,7 +80,7 @@ jobs:
cmake ../examples -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX=~/local -DZENOHC_SOURCE=PACKAGE
cmake --build . --config Release

- name: Run tests
- name: Run rust tests
uses: actions-rs/cargo@v1
with:
command: test
Expand Down
7 changes: 7 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,15 @@ add_library(zenohc::static ALIAS zenohc_static)
add_library(zenohc::lib ALIAS zenohc)
add_dependencies(zenohc_static cargo)
add_dependencies(zenohc cargo)

#
# Setup additional properties for library targets
# *IMPORTANT* any options in this section should be repeated in install/PackageConfig.cmake.in
# to achieve correct behavior of find_package(zenohc)
#
get_required_static_libs(NATIVE_STATIC_LIBS)
target_link_libraries(zenohc_static INTERFACE ${NATIVE_STATIC_LIBS})
target_compile_definitions(zenohc INTERFACE ZENOHC_DYN_LIB)

# Workaroud for https://github.com/rust-lang/cargo/issues/5045
# mentioned in https://github.com/eclipse-zenoh/zenoh-c/issues/138
Expand Down
44 changes: 30 additions & 14 deletions build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@ fn main() {
.write_to_file(GENERATION_PATH);

configure();
split_bindings().unwrap();
rename_enums();
let split_guide = SplitGuide::from_yaml(SPLITGUIDE_PATH);
split_bindings(&split_guide).unwrap();
text_replace(split_guide.rules.iter().map(|(name, _)| name.as_str()));

println!("cargo:rerun-if-changed=build.rs");
println!("cargo:rerun-if-changed=src");
Expand Down Expand Up @@ -42,9 +43,8 @@ fn configure() {
file.unlock().unwrap();
}

fn rename_enums() {
let split_guide = SplitGuide::from_yaml(SPLITGUIDE_PATH);
for (name, _) in split_guide.rules.iter() {
fn text_replace<'a>(files: impl Iterator<Item = &'a str>) {
for name in files {
let path = format!("include/{}", name);

// Read content
Expand All @@ -60,9 +60,13 @@ fn rename_enums() {
file.unlock().unwrap();

// Remove _T_ from enum variant name
let new = buf.replace("_T_", "_");
let buf = buf.replace("_T_", "_");
// Replace _t_Tag from union variant name
let new = new.replace("_t_Tag", "_tag_t");
let buf = buf.replace("_t_Tag", "_tag_t");
// Insert `ZENOHC_API` macro before `extern const`.
// The cbindgen can prefix functions (see `[fn] prefix=...` in cbindgn.toml), but not extern variables.
// So have to do it here.
let buf = buf.replace("extern const", "ZENOHC_API extern const");

// Overwrite content
let mut file = std::fs::File::options()
Expand All @@ -73,13 +77,12 @@ fn rename_enums() {
.open(&path)
.unwrap();
file.lock_exclusive().unwrap();
file.write_all(new.as_bytes()).unwrap();
file.write_all(buf.as_bytes()).unwrap();
file.unlock().unwrap();
}
}

fn split_bindings() -> Result<(), String> {
let split_guide = SplitGuide::from_yaml(SPLITGUIDE_PATH);
fn split_bindings(split_guide: &SplitGuide) -> Result<(), String> {
let bindings = std::fs::read_to_string(GENERATION_PATH).unwrap();
let mut files = split_guide
.rules
Expand All @@ -97,7 +100,10 @@ fn split_bindings() -> Result<(), String> {
(name.as_str(), BufWriter::new(file))
})
.collect::<HashMap<_, _>>();
let mut records = group_tokens(Tokenizer { inner: &bindings })?;
let mut records = group_tokens(Tokenizer {
filename: GENERATION_PATH,
inner: &bindings,
})?;
for id in split_guide.requested_ids() {
if !records.iter().any(|r| r.contains_id(id)) {
return Err(format!(
Expand Down Expand Up @@ -303,6 +309,7 @@ impl<'a> Record<'a> {
TokenType::PreprDefine => self.push_record_type_token(token, RecordType::PreprDefine),
TokenType::PreprInclude => self.push_record_type_token(token, RecordType::PreprInclude),
TokenType::PreprIf => self.push_prepr_if(token),
TokenType::PreprElse => self.push_token(token),
TokenType::PreprEndif => self.push_prepr_endif(token)?,
TokenType::Whitespace => self.push_token(token),
}
Expand Down Expand Up @@ -339,6 +346,7 @@ enum TokenType {
PreprDefine,
PreprInclude,
PreprIf,
PreprElse,
PreprEndif,
Whitespace,
}
Expand Down Expand Up @@ -375,6 +383,7 @@ impl<'a> Token<'a> {
.or_else(|| Self::prepr_include(s))
.or_else(|| Self::prepr_define(s))
.or_else(|| Self::prepr_if(s))
.or_else(|| Self::prepr_else(s))
.or_else(|| Self::typedef(s))
.or_else(|| Self::r#const(s))
.or_else(|| Self::function(s))
Expand Down Expand Up @@ -517,11 +526,16 @@ impl<'a> Token<'a> {
.then(|| Token::new(TokenType::PreprEndif, "", s.until("\n").unwrap_or(s)))
}

fn prepr_else(s: &'a str) -> Option<Self> {
s.starts_with("#else")
.then(|| Token::new(TokenType::PreprElse, "", s.until("\n").unwrap_or(s)))
}

fn prepr_include(s: &'a str) -> Option<Self> {
Self::_include(s, "#include \"", "\"").or_else(|| Self::_include(s, "#include <", ">"))
Self::r#include(s, "#include \"", "\"").or_else(|| Self::r#include(s, "#include <", ">"))
}

fn _include(s: &'a str, start: &str, end: &str) -> Option<Self> {
fn r#include(s: &'a str, start: &str, end: &str) -> Option<Self> {
if s.starts_with(start) {
let span = s.until_incl(end).expect("detected unterminated #include");
Some(Token::new(
Expand All @@ -547,6 +561,7 @@ impl Until for &str {
}
}
struct Tokenizer<'a> {
filename: &'a str,
inner: &'a str,
}
impl<'a> Iterator for Tokenizer<'a> {
Expand All @@ -561,7 +576,8 @@ impl<'a> Iterator for Tokenizer<'a> {
self.inner = &self.inner[result.span.len()..];
} else {
panic!(
"Couldn't parse C file, stopped at: {}",
"Couldn't parse C file {}, stopped at: {}",
self.filename,
self.inner.lines().next().unwrap()
)
}
Expand Down
2 changes: 1 addition & 1 deletion cbindgen.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ renaming_overrides_prefixing = false
[fn]
rename_args = "None"
# must_use = "MUST_USE_FUNC"
# prefix = "START_FUNC"
prefix = "ZENOHC_API"
# postfix = "END_FUNC"
args = "auto"
sort_by = "Name"
Expand Down
6 changes: 6 additions & 0 deletions include/zenoh.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ extern "C" {
#define ALIGN(n) __attribute__((aligned(n)))
#endif

#if defined(ZENOHC_DYN_LIB) && defined(_MSC_VER)
#define ZENOHC_API __declspec(dllimport)
#else
#define ZENOHC_API
#endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a good trick for libraries that use cmake build system. You can control your export definition by distinguishing between PRIVATE and INTERFACE includes:

macro(target_define_export TARGET_NAME)
if(${WIN32})
	target_compile_definitions(${TARGET_NAME} PRIVATE ${TARGET_NAME}_EXPORT=__declspec\(dllexport\))
	target_compile_definitions(${TARGET_NAME} INTERFACE ${TARGET_NAME}_EXPORT=__declspec\(dllimport\))
else()
    target_compile_definitions(${TARGET_NAME} PUBLIC ${TARGET_NAME}_EXPORT=)
endif()
endmacro()

It isalso possible to handle static library generation here by checking target type:
get_target_property(target_type <target> TYPE)
It will be one of STATIC_LIBRARY, MODULE_LIBRARY, SHARED_LIBRARY, EXECUTABLE or one of the internal target types.

I do not insist on this approach, but it looks like a cleaner cmake-way to define library exports.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this approach is applicable ONLY if your library is being integrated into other projects as source- cmake package

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this approach is applicable ONLY if your library is being integrated into other projects as source- cmake package

Yes, and that's why I think this approach is not suitable. I believe that it's bad practice to place any pieces of code into cmake files. User should be able to integrate library into his build system without using of our cmake scripts. I.e. from my point of view:

  • setting compiler definitions with CMake - OK
  • assigning real pieces of code to variables (like __declspec(dllimport) with CMAke - Wrong

#include "zenoh_configure.h"

// clang-format off
Expand Down
Loading