Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
encoding/json: Add custom JSON package with build tag support for Sonic #1623
encoding/json: Add custom JSON package with build tag support for Sonic #1623
Changes from 28 commits
8333e5c
c47137c
6e24f60
b70b9e7
84dd409
711db77
223f786
9a72688
2f69362
79c85c9
b4f0243
417e0aa
83f1d57
f2d56cf
7bd7cc4
79ebfa1
6b6fb5e
ac47cc6
135e2b6
2d08549
1a82764
1ca8eaa
ab5fd01
1697977
3905b62
1c9f9c2
4f8e487
9cffc8f
1b15e4c
a7ac15d
f23f3c3
fba5316
880cd37
dde84c7
c936252
a843b58
09bac6c
a9ce5aa
625ae13
9fff614
b8a5070
9a52e19
9c154ca
ce01376
fcc0b27
d49cf8e
ad20589
c1f1216
00ebc20
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is different from all previous examples.
You're not building sonic. You're building gct with sonic.
And it leaves a gap where install should be.
I suggest that users need to use
make build
andinstall
with an argument:Or, if we go the route I'm proposing:
Also: Given the recent highlighting of
act
, I wonder if we remove Makefile and update docs to useact
, and make that a (non-runtime) dependency for installing GCT from source.Note: As we approach semver (right?), we should be issuing binaries anyway
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.
If we are going to allow customisation, I think a more general approach
make build FLAGS="-tags=sonic"
might be better. This gives users the flexibility to specify any build tags or flags they need without hardcoding specific options in the Makefile. If users have to manually specify flags every time, they might as well use go build or go install directly. The goal of make is to simplify workflows, not add unnecessary complexity.We could:
Keep the Makefile but allow FLAGS to be overridden for advanced use cases.
Provide default targets like make build-sonic for common configurations.
As for act that is an option. But I have little to no experience in semver and issuing binaries so you can discuss with @thrasher- && @gloriousCode
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.
If you're going to use FLAGS, then:
makes more sense than adding it to the end, and means no extra support for FLAGS in Makefile.
( Note: I'll accept nosonic or sonicless )
act: I've only just started using it
Semver is a separate topic, and issuing binaries doesn't need to be related to it, now that I think about it.
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.
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.
Please keep styling in line with the others (or update the others) and update the template as well.