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: hybird compilation issue on windows #179

Merged
merged 5 commits into from
Apr 15, 2024

Conversation

ShyunnY
Copy link
Contributor

@ShyunnY ShyunnY commented Apr 12, 2024

Fix

Signed-off-by: ShyunnY <1147212064@qq.com>
@mrproliu mrproliu self-requested a review April 12, 2024 03:05
@mrproliu mrproliu added the bug Something isn't working label Apr 12, 2024
@mrproliu mrproliu added this to the 0.5.0 milestone Apr 12, 2024
Copy link
Contributor

@mrproliu mrproliu left a comment

Choose a reason for hiding this comment

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

Thanks! Could you please also update the CHANGES.md. Also, would you help to add a new plugin test in GHA for testing whether the go agent works on the windows or not?

go.work.sum Outdated Show resolved Hide resolved
@wu-sheng
Copy link
Member

Do we have a Windows to verify?

@ShyunnY
Copy link
Contributor Author

ShyunnY commented Apr 12, 2024

我们有 Windows 来验证吗?

I will write e2e tests :)

@mrproliu
Copy link
Contributor

Will you write in this PR or a new one? @ShyunnY

@ShyunnY
Copy link
Contributor Author

ShyunnY commented Apr 12, 2024

Will you write in this PR or a new one? @ShyunnY

Since it may be a while before I can write the e2e tests, I thought I'd start this work in a new PR.
Thank you for your review! :)

@wu-sheng
Copy link
Member

If CI can't be added here, we need a manul test. @mrproliu

@mrproliu
Copy link
Contributor

Could you help to add some screenshot in here? @ShyunnY

@ShyunnY
Copy link
Contributor Author

ShyunnY commented Apr 12, 2024

cc @wu-sheng

Hello! When I was fixing issue-12068, I found that go-agent had other problems, which made it impossible to carry out hybrid compilation under the windows platform.
I think this PR is only to fix the problem that go-agent can work normally under the Windows platform, not to solve the problem of issue-12097.

IMO, I think we should merge this PR first, and I will submit a new PR to solve the issue-12097 problem in the future.

By the way, E2E testing should be carried out after we fix all issues. :)

@wu-sheng
Copy link
Member

To be honest, as you are fixing Windows compiling, the new resolved issue should be fixed as well.

And this PR has CI pipeline failing, which can't be merged by anyone, and doesn't fit SkyWalking traditions.

@wu-sheng
Copy link
Member

You could use one PR to fix two issues, which is fine.
The expectation in SkyWalking is, we prefer PR resolves known relative issues once for all.

@ShyunnY
Copy link
Contributor Author

ShyunnY commented Apr 12, 2024

@wu-sheng

Sorry, since this is my first time participating in a skywalking project, I may not be familiar with the skywalking style, but today I understand it.

Thanks for your reply, I will work on this as soon as possible.

@Superskyyy
Copy link
Member

One can add windows Github action to the .github workflows, just I'm not sure if our e2e pipeline can work on windows.

@wu-sheng
Copy link
Member

I don't think e2e tool works. But at least, we could have compiling passed on Windows CI.

Signed-off-by: ShyunnY <1147212064@qq.com>
Copy link
Contributor

@mrproliu mrproliu left a comment

Choose a reason for hiding this comment

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

LGTM

@wu-sheng
Copy link
Member

I can't see Windows platform CI compiling.

@mrproliu
Copy link
Contributor

Could you upload some screenshots here? @ShyunnY

@mrproliu
Copy link
Contributor

@ShyunnY basic finished the compile phase on windows, but the tracing data not reported for now, he is working on it.

@ShyunnY
Copy link
Contributor Author

ShyunnY commented Apr 15, 2024

Follow the steps of docs. We expect it to execute normally under windows.

actual:
image

Project binaries successfully compile

@wu-sheng wu-sheng merged commit 83604b9 into apache:main Apr 15, 2024
38 checks passed
@ShyunnY ShyunnY deleted the hybird-compile-win branch April 16, 2024 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants