-
-
Notifications
You must be signed in to change notification settings - Fork 937
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
feat: build of shared/static libraries #3511
Conversation
looking good so far! Thank you! I added a couple of comments, and regarding your question about the archive, you'll need to add the Header artifact type in the filter there:
There might be more places, so you can probably search for Another thing that comes to mind is that we have the That's all I remember for now about this. BTW feel free to ping me if you need any help, I'm mostly available on discord for quick chats if needed 🙏 And thanks again for your effort and work on this 🤘 |
Codecov Report
@@ Coverage Diff @@
## main #3511 +/- ##
==========================================
- Coverage 84.27% 83.88% -0.40%
==========================================
Files 114 114
Lines 9338 9403 +65
==========================================
+ Hits 7870 7888 +18
- Misses 1191 1232 +41
- Partials 277 283 +6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Thanks for the review @caarlos0 ! I replied to the comments best I could. As for the other points:
I added this line here: https://github.com/goreleaser/goreleaser/pull/3511/files#diff-4b69e49926d4bcaf08f5563b284ff33ac8f7a60a596dc62898ec333253014088R94 Did I miss it in other places though?
I need to think this through: the interesting exception in that scenario would be that the header file looks always the same, so it could be uploaded just once: it is not specific to any target os/arch combo. |
996cef2
to
b267970
Compare
I think maybe nfpm if we want to create library packages... but that might be a bigger effort (and maybe worth doing in another pr)
hmmm that seems like a bigger change too.. maybe for now we just assume that libraries will always be archived? |
I guess that makes sense.. then the only point missing should then be to create 1 or 2 new Artefact types "DynamicLibrary"/"StaticLibrary" for c-shared and c-archive respectively (or just 1 "Library" for both? wdyt?) Anything else I may be missing? |
I think we can create both 2 new types, in case we need some extra handling somewhere... I can't think of anything else missing besided that and the nfpm/brew/etc stuff (which we can handle in the future) |
b267970
to
42d3ed6
Compare
Given the latest comments, to more easily support the 2 possible artefacts, and considering that the field would not be templateable anyway, I tried to refactor things a bit. I have introduced a I kept it in a separate commit but can squash later if you like a957da6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a single comment, I believe we are very close!
Thank you a ton for this!
62da019
to
b4612d8
Compare
* add the correct extension according to the lib type and target os * archive the generated header file
instead of parsing the flags, use a buildmode option to configure C shared and archive libs
b4612d8
to
4afae2f
Compare
So, I did now a rebase to main, and tested it further on the field. Now when setting the buildmode we always fall back to Additionally I found and fixed another bug with the override logic. |
By default use .a and .so for static and shared libs respectively. On macOS/Darwin - use .dylib for shared libraries. On Windows - use .lib for static and .dll for dynamic libs.
bc67de0
to
e8de882
Compare
This looks awesome, thank you! |
<!-- Hi, thanks for contributing! Please make sure you read our CONTRIBUTING guide. Also, add tests and the respective documentation changes as well. --> <!-- If applied, this commit will... --> This PR improves the handling of shared or static libraries by GoReleaser. It uses the default behaviour of the Go compiler by appending the right extension to libraries. * `.so` and `.a` for Linux shared libraries and static libraries respectively * `.dylib` and `.a.` on Darwin * `.dll` and `.lib` on Windows (pre-existent) It does not add any configuration option to `.goreleaser.yml`, since it leverages the existing `buildmode` flag. Additionally, this PR takes care of adding the generated header file into the archive. <!-- Why is this change being made? --> Personally I would leverage this change to release some software both as a CLI and as a shared library. I believe others who use CGo or need interoperability with Go from other languages could benefit from this. <!-- # Provide links to any relevant tickets, URLs or other resources --> This was previously discussed in goreleaser#3497. I couldn't quite think of a proper way to add some tests to the header archiving feature. Any recommendation?
This PR improves the handling of shared or static libraries by GoReleaser. It uses the default behaviour of the Go compiler by appending the right extension to libraries.
.so
and.a
for Linux shared libraries and static libraries respectively.dylib
and.a.
on Darwin.dll
and.lib
on Windows (pre-existent)It does not add any configuration option to
.goreleaser.yml
, since it leverages the existingbuildmode
flag.Additionally, this PR takes care of adding the generated header file into the archive.
Personally I would leverage this change to release some software both as a CLI and as a shared library. I believe others who use CGo or need interoperability with Go from other languages could benefit from this.
This was previously discussed in #3497.
I couldn't quite think of a proper way to add some tests to the header archiving feature. Any recommendation?