Skip to content

feat: Explicitly mark exported API#437

Merged
lan496 merged 4 commits intospglib:developfrom
LecrisUT:feat/export
Feb 21, 2024
Merged

feat: Explicitly mark exported API#437
lan496 merged 4 commits intospglib:developfrom
LecrisUT:feat/export

Conversation

@LecrisUT
Copy link
Copy Markdown
Collaborator

@LecrisUT LecrisUT commented Feb 15, 2024

Windows systems are quite weird where you have to define both when you import and export stuff. Previously in the CI we got around it by building as static, but that is not appropriate as we've seen with the C# interface discussion 1.

@husakm can you try this one out?

Footnotes

  1. https://github.com/spglib/spglib/discussions/399#discussioncomment-8207195 ↩

@LecrisUT LecrisUT added the bug label Feb 15, 2024
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 15, 2024 •
edited
Loading

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (8da9279) 83.59% compared to head (c21cda3) 79.70%.

Files Patch % Lines
src/spglib.c 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     #437      +/-   ##
===========================================
- Coverage    83.59%   79.70%   -3.90%     
===========================================
  Files           24       23       -1     
  Lines         8119     6213    -1906     
  Branches      1694     1633      -61     
===========================================
- Hits          6787     4952    -1835     
+ Misses        1332     1261      -71
Flag Coverage Δ
c_api ?
fortran_api ?
python_api 79.70% <0.00%> (-0.05%) ⬇️
unit_tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LecrisUT LecrisUT force-pushed the feat/export branch 13 times, most recently from c63a969 to 32d8def Compare February 15, 2024 19:41
@LecrisUT LecrisUT self-assigned this Feb 15, 2024
@LecrisUT LecrisUT force-pushed the feat/export branch 8 times, most recently from 950f2eb to ce7911c Compare February 15, 2024 21:38
@LecrisUT LecrisUT marked this pull request as ready for review February 15, 2024 21:41
@LecrisUT LecrisUT mentioned this pull request Feb 15, 2024
Closed
5 tasks
@LecrisUT LecrisUT requested review from atztogo and lan496 February 15, 2024 21:44
@LecrisUT LecrisUT removed their assignment Feb 15, 2024
@atztogo
Copy link
Copy Markdown
Collaborator

atztogo commented Feb 16, 2024

@LecrisUT, could you tell me how the macros in base.h work or the aim unless it is written somewhere?

@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Feb 16, 2024

Basically the default symbol visibility is changed to hidden, but that means the unit tests would not be able to find the symbols, even though it has access to the internal headers. So, as a workaround, we patch the logic to export those symbols when the test-suite is being built with unit tests.

The other approaches I've seen are:

  • build the static library together with the shared one. This has a lot of overhead downstream because they would have to run the configure-build-test twice
  • make maintenance hatch exposed interfaces, i.e. a wrapper that exposes the api and with test nomenclature. Since the headers are not public, the current approach effectively makes them maintenance hatch interface, so no need to be explicit

atztogo
atztogo approved these changes Feb 16, 2024
View reviewed changes
src/base.h
#include "spglib.h"

#ifdef SPG_TESTING
#define SPG_API_TEST SPG_API
Copy link
Copy Markdown
Collaborator

@atztogo atztogo Feb 16, 2024

Choose a reason for hiding this comment

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

Where is SPG_API used?

Copy link
Copy Markdown
Collaborator Author

@LecrisUT LecrisUT Feb 16, 2024

Choose a reason for hiding this comment

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

It's defined in spglib.h and it's used to export/import funtions to/from dll. On unix it only does export

@LecrisUT LecrisUT mentioned this pull request Feb 16, 2024
Open
@lan496
Copy link
Copy Markdown
Member

lan496 commented Feb 16, 2024 •
edited
Loading

Can you leave a document on SPG_API_TEST in Contributing.md or somewhere appropriate? Small note on why this macro is needed will be enough.

@LecrisUT LecrisUT mentioned this pull request Feb 17, 2024
Closed
@LecrisUT
Copy link
Copy Markdown
Collaborator Author

LecrisUT commented Feb 21, 2024

Ok, I've added a note about SPG_API_TEST in test/README.md. I think we should move it when we refactor the documentation structure.

lan496
lan496 approved these changes Feb 21, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@lan496 lan496 left a comment

Choose a reason for hiding this comment

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

Thanks. The note Exposing API for unit tests looks concise and simple for contributors.

@lan496 lan496 merged commit f1ff014 into spglib:develop Feb 21, 2024
@LecrisUT LecrisUT deleted the feat/export branch February 26, 2024 08:12
@LecrisUT LecrisUT mentioned this pull request Feb 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@atztogo atztogo atztogo approved these changes

@lan496 lan496 lan496 approved these changes

Assignees

@lan496 lan496

Labels

bug

Projects

Spglib developers' kanban
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LecrisUT @atztogo @lan496