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

feat: code templates can declare required go langage level #357

Merged
merged 10 commits into from
Oct 30, 2024

Conversation

RomainMuller
Copy link
Contributor

In certain cases, it is not practical to write aspects using go syntax that is known to work with all go versions. While this is in general not an issue (since orchestrion only supports the latest 2 go releases), it can still cause issues when injecting code into modules with a go.mod file that declares an antiquated go version (e.g: go 1.13).

In such cases, go 1.22 and newer toolchains will instruct the compiler to process the source code in a way that ensures compatibility with that antiquated language level, prohibiting use of newer language features.

To protect ourselves from this issue, the template objects can now be configured to demand a certain minimum language level to be available, which will cause compile units into which that template generated code to have the compiler's -lang flag set to that level (unless it was already set at a higher level).

This might have consequences on the behavior of the compiled code (e.g, the change of for range variable bindings introduced in go 1.22); but we currently do not deal with this particular issue, as we assume it will seldom have any practical impact in our use-cases.

In certain cases, it is not practical to write aspects using go syntax
that is known to work with all go versions. While this is in general not
an issue (since `orchestrion` only supports the latest 2 go releases),
it can still cause issues when injecting code into modules with a
`go.mod` file that declares an antiquated `go` version (e.g: `go 1.13`).

In such cases, go 1.22 and newer toolchains will instruct the compiler
to process the source code in a way that ensures compatibility with that
antiquated language level, prohibiting use of newer language features.

To protect ourselves from this issue, the `template` objects can now be
configured to demand a certain minimum language level to be available,
which will cause compile units into which that template generated code
to have the compiler's `-lang` flag set to that level (unless it was
already set at a higher level).

This might have consequences on the behavior of the compiled code (e.g,
the change of `for range` variable bindings introduced in go 1.22); but
we currently do not deal with this particular issue, as we assume it
will seldom have any practical impact in our use-cases.
@RomainMuller RomainMuller requested a review from a team as a code owner October 24, 2024 15:45
@RomainMuller RomainMuller changed the title feat: code templates can declare require go langage level feat: code templates can declare required go langage level Oct 25, 2024
Copy link
Contributor

@rarguelloF rarguelloF left a comment

Choose a reason for hiding this comment

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

Overall LGTM, just working with the go version strings can be simplified using go/version package instead of golang.org/x/mod/semver.

@@ -44,15 +44,29 @@ imports:
tracer: gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer
```

### Links
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is Links disappearing? is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was never there to begin with! It's only supported for inject-declarations...

@RomainMuller RomainMuller force-pushed the romain.marcadier/template-go-lang branch from e69e70c to 9910926 Compare October 30, 2024 10:30
@RomainMuller RomainMuller added this pull request to the merge queue Oct 30, 2024
Merged via the queue into main with commit 70d4d28 Oct 30, 2024
25 checks passed
@RomainMuller RomainMuller deleted the romain.marcadier/template-go-lang branch October 30, 2024 15:36
Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 78.86179% with 26 lines in your changes missing coverage. Please review.

Project coverage is 58.63%. Comparing base (aa25c54) to head (28c3451).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
internal/toolexec/proxy/compile.go 63.15% 6 Missing and 1 partial ⚠️
internal/injector/aspect/context/golang.go 81.81% 5 Missing and 1 partial ⚠️
internal/injector/aspect/advice/code/template.go 58.33% 2 Missing and 3 partials ⚠️
internal/injector/injector.go 87.50% 3 Missing ⚠️
internal/toolexec/aspect/oncompile.go 25.00% 2 Missing and 1 partial ⚠️
internal/toolexec/proxy/proxy.go 83.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #357      +/-   ##
==========================================
+ Coverage   58.50%   58.63%   +0.13%     
==========================================
  Files         158      159       +1     
  Lines       11333    11423      +90     
==========================================
+ Hits         6630     6698      +68     
- Misses       4232     4249      +17     
- Partials      471      476       +5     
Components Coverage Δ
Generators 76.98% <ø> (ø)
Instruments 88.05% <ø> (ø)
Go Driver 73.13% <ø> (ø)
Toolexec Driver 70.64% <65.71%> (-0.24%) ⬇️
Aspects 71.16% <82.81%> (+0.31%) ⬆️
Injector 73.33% <84.09%> (+0.19%) ⬆️
Job Server 63.45% <ø> (ø)
Integration Test Suite 49.82% <ø> (ø)
Other 58.63% <78.86%> (+0.13%) ⬆️
Files with missing lines Coverage Δ
internal/injector/aspect/advice/assign.go 68.96% <100.00%> (+2.29%) ⬆️
internal/injector/aspect/advice/block.go 70.00% <100.00%> (+2.14%) ⬆️
internal/injector/aspect/advice/call.go 75.88% <100.00%> (+0.17%) ⬆️
internal/injector/aspect/advice/inject.go 74.41% <100.00%> (+1.24%) ⬆️
internal/injector/aspect/advice/wrap.go 75.67% <100.00%> (+1.38%) ⬆️
internal/injector/aspect/context/context.go 97.14% <100.00%> (+0.17%) ⬆️
internal/toolexec/proxy/proxy.go 67.56% <83.33%> (+3.05%) ⬆️
internal/injector/injector.go 74.28% <87.50%> (+0.75%) ⬆️
internal/toolexec/aspect/oncompile.go 72.51% <25.00%> (-1.70%) ⬇️
internal/injector/aspect/advice/code/template.go 62.27% <58.33%> (-0.85%) ⬇️
... and 2 more

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