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

Prepare repo for use #4

Merged
merged 14 commits into from
Oct 4, 2024
Merged

Prepare repo for use #4

merged 14 commits into from
Oct 4, 2024

Conversation

mjwolf
Copy link
Collaborator

@mjwolf mjwolf commented Oct 2, 2024

Prepare the repo for use by:

  • Add .go files currently in github.com/elastic/quark
  • Adding license and notice files
  • Set up submodule with github.com/elastic/quark
  • Setting up build scripts

Note on the build system:
To build a usable Go module, which doesn't require users to compile C code before themselves, compiled libquark artifacts are included in the published Go module. These files must be compiled by CI machines. In order to enforce this, any commits with modified .a files will have an automated commit overwrite these changes with files compiled by CI machines, so any merged PRs only contain .a files created by build machines.

@mjwolf mjwolf requested a review from a team as a code owner October 2, 2024 21:23
@mjwolf mjwolf marked this pull request as draft October 2, 2024 21:35
@mjwolf mjwolf removed the request for review from a team October 2, 2024 21:35
@mjwolf mjwolf force-pushed the test-builds branch 2 times, most recently from c54c63c to 12c98f1 Compare October 2, 2024 22:02
.gitignore Outdated
Comment on lines 1 to 3
# .a files should not be committed by users; automation will build and commit
# changes in PR branches.
*.a
Copy link
Member

Choose a reason for hiding this comment

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

I guess these .a files have to be committed? 😬

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, they do. quark is a C library, and the .a files are the compiled version of it. For Go modules, including the compiled lib is standard, see https://github.com/richardwilkes/unison/tree/main/internal/skia and https://github.com/periph/d2xx/tree/main/third_party.

The only other alternative would be to have the users of the Go library compile the C code themselves, which would mean all users would need to set up a C toolchain and build system, and it would just be much more difficult for anyone to use this

bin/go.mod Outdated

go 1.22

replace github.com/elastic/go-quark => ../
Copy link
Member

Choose a reason for hiding this comment

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

FYI if we're expecting folks to go install this repo, this won't work - see also StyraInc/regal#490

We shouldn't need this replace at all? I'm not sure we need an additional module here, but could instead have folks install i.e. via

go install github.com/elastic/go-quark/bin@latest
# preferably
go install github.com/elastic/go-quark/cmd/quark@latest

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, I didn't know that.

These files were moved from a different directory structure, and not actually working yet, but that will help me to get it working

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can forego the binary under bin, this was basically just a test thing I had to get things going, in the future I might clean-up and make it usable, the "actual" exported quark-mon is in C and it's here: https://elastic.github.io/quark/quark-mon.8.html
https://github.com/elastic/quark/blob/main/quark-mon.c

So I'd leave the bin/quak-mon.go outside the build system in this repository

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've removed quark-mon.go for now

@mjwolf mjwolf marked this pull request as ready for review October 4, 2024 20:26
@mjwolf mjwolf changed the title add submodule Prepare repo for use Oct 4, 2024
@mjwolf mjwolf merged commit 366748a into main Oct 4, 2024
1 check passed
@mjwolf mjwolf deleted the test-builds branch October 4, 2024 20:47
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