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

Fix undefined symbol errors on building otelcorecol for Plan 9 #6924

Merged
merged 1 commit into from
Jan 18, 2023

Conversation

lufia
Copy link
Contributor

@lufia lufia commented Jan 10, 2023

Description:

Building otelcorecol was broken for Plan 9.

$ cd cmd/otelcorecol
$ GOOS=plan9 go build
# go.opentelemetry.io/collector/exporter/loggingexporter
../../exporter/loggingexporter/known_sync_error.go:29:10: undefined: syscall.ENOTSUP
../../exporter/loggingexporter/known_sync_error.go:31:10: undefined: syscall.ENOTTY
../../exporter/loggingexporter/known_sync_error.go:33:10: undefined: syscall.EBADF

Testing:
Though I'm not sure yet there are more other issues when otelcorecol is running on Plan 9, at least, this PR is fixed undefined symbol errors and I checked otelcorecol shows a help successfully with -h option.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 10, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: lufia / name: KADOTA, Kyohei (c03b161)

@lufia lufia marked this pull request as ready for review January 10, 2023 08:43
@lufia lufia requested review from a team and codeboten January 10, 2023 08:43
@lufia lufia force-pushed the otelcol-plan9build branch from c03b161 to 30939a1 Compare January 12, 2023 10:00
@codecov
Copy link

codecov bot commented Jan 16, 2023

Codecov Report

Base: 91.05% // Head: 91.08% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (a8af06c) compared to base (7d168dd).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6924      +/-   ##
==========================================
+ Coverage   91.05%   91.08%   +0.03%     
==========================================
  Files         237      238       +1     
  Lines       14298    14418     +120     
==========================================
+ Hits        13019    13133     +114     
- Misses       1027     1033       +6     
  Partials      252      252              
Impacted Files Coverage Δ
exporter/loggingexporter/known_sync_error.go 0.00% <ø> (ø)
connector/connector.go 100.00% <100.00%> (ø)
connector/connectortest/connector.go 100.00% <100.00%> (ø)
service/extensions/extensions.go 77.90% <0.00%> (-0.99%) ⬇️
service/pipelines.go 93.93% <0.00%> (-0.30%) ⬇️
service/internal/components/loggers.go 76.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Hey @lufia, thanks for the PR. While we don't currently have a well defined policy on when/how to add support to new OS or architectures, I personally would be fine accepting this change with the understanding that

  • we can't ensure we won't break the Plan 9 build in the future
  • we probably will not be able to address any bug reports that are Plan 9 specific
  • we won't provide official builds or run the test suite on CI for Plan 9
  • a Plan 9 build may not have feature parity with our official builds
  • If there are issues that interfere with development we may need to remove support for Plan 9 entirely if we don't get help from you or other contributors

(where 'we' here is to be understood as 'the current OTel Collector community')

The above is my personal take on this, other contributors may disagree. I can work on getting agreement from the community and making this informal policy more well defined if this seems acceptable to you.

exporter/loggingexporter/known_sync_error.go Outdated Show resolved Hide resolved
@lufia lufia force-pushed the otelcol-plan9build branch from 30939a1 to 9879a6c Compare January 17, 2023 11:55
@lufia
Copy link
Contributor Author

lufia commented Jan 17, 2023

Hi @mx-psi thank you for your review!

I have agree all of them.

I'd like to serve otelcontribcol service in my own Plan 9 box so far. I would make an effort to keep Plan 9 build healthy as far as I possible so please feel free to tell me if someone notices any issues in the future.

@lufia lufia force-pushed the otelcol-plan9build branch from 9879a6c to a8af06c Compare January 17, 2023 12:19
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

LGTM 🐰 🧑‍🚀

@lufia
Copy link
Contributor Author

lufia commented Jan 17, 2023

Thats glenda!

// See the License for the specific language governing permissions and
// limitations under the License.

//go:build plan9
Copy link
Member

Choose a reason for hiding this comment

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

should this be "other" file and have the rule !linux && !darwin?

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