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

[MINOR] Fix ci check error #4543

Merged
merged 7 commits into from
Nov 9, 2023
Merged

[MINOR] Fix ci check error #4543

merged 7 commits into from
Nov 9, 2023

Conversation

xwm1992
Copy link
Contributor

@xwm1992 xwm1992 commented Nov 6, 2023

Fixes #.

Motivation

Explain the content here.
Explain why you want to make the changes and what problem you're trying to solve.

Modifications

Describe the modifications you've done.

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Merging #4543 (5557311) into master (c350528) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 5557311 differs from pull request most recent head 53715a6. Consider uploading reports for the commit 53715a6 to get more accurate results

@@             Coverage Diff              @@
##             master    #4543      +/-   ##
============================================
+ Coverage     16.28%   16.30%   +0.02%     
- Complexity     1591     1592       +1     
============================================
  Files           747      747              
  Lines         28954    28965      +11     
  Branches       2525     2541      +16     
============================================
+ Hits           4715     4723       +8     
- Misses        23790    23793       +3     
  Partials        449      449              

see 13 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

- if: matrix.language == 'cpp' || matrix.language == 'csharp'
name: Build C
run: |
make
Copy link
Member

Choose a reason for hiding this comment

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

The error 'CodeQL detected code written in Java/Kotlin, Go and c, but not any written in C/C++' is still reported in CI. I saw that the CI result shows that this step has been skipped. Is it because this judgment condition requires the addition of matrix. language=='c'?


CI仍然报这个错"CodeQL detected code written in Java/Kotlin, Go and c, but not any written in C/C++"。我看CI结果显示这一步跳过了,是不是因为这个判断条件需要加上matrix.language =='c'

Copy link
Contributor Author

@xwm1992 xwm1992 Nov 6, 2023

Choose a reason for hiding this comment

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

The error 'CodeQL detected code written in Java/Kotlin, Go and c, but not any written in C/C++' is still reported in CI. I saw that the CI result shows that this step has been skipped. Is it because this judgment condition requires the addition of matrix. language=='c'?

CI仍然报这个错"CodeQL detected code written in Java/Kotlin, Go and c, but not any written in C/C++"。我看CI结果显示这一步跳过了,是不是因为这个判断条件需要加上matrix.language =='c'

cpp is c/c++, and there are language settings to display it in CodeQL


cpp就是c/c++,CodeQL里面有显示它的语言设置

Copy link
Contributor Author

@xwm1992 xwm1992 Nov 6, 2023

Choose a reason for hiding this comment

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

xwm1992#11

This is the pr modification of my own repository. It shows that the ci check passed, but the submission to the apache repository failed.


这是我自己仓库的pr修改,显示ci check是通过了的,但是提交到主仓库就失败了

Copy link
Member

@Pil0tXia Pil0tXia Nov 6, 2023

Choose a reason for hiding this comment

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

but the submission to the apache repository failed.

It should be a side effect of these unsynchronized commits: xwm1992/EventMesh@master...apache:eventmesh:master


应该是这些没同步的commit带来的副作用:xwm1992/EventMesh@master...apache:eventmesh:master

Copy link
Member

@pandaapo pandaapo Nov 6, 2023

Choose a reason for hiding this comment

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

xwm1992#11
This is the pr modification of my own repository. It shows that the ci check passed, but the submission to the apache repository failed.

After comparing the CI information of the two repositories, there are still differences. The former's CodeQL seems to only analyze Java, while the latter is not. But I am not sure about the specific reason.

捕获0
捕获


对比了一下两个仓库的CI信息,还是有差别的。前者的CodeQL好像只分析java的,后者则不是。但具体原因我也不清楚。

Copy link
Member

@mxsm mxsm Nov 7, 2023

Choose a reason for hiding this comment

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

xwm1992#11
This is the pr modification of my own repository. It shows that the ci check passed, but the submission to the apache repository failed.

会不会跟make的路径有关,在根目录下面不能进行make 编译处理C而需要到sdk Makefile文件目录才能


Could it be related to the path of make? Compilation processing of C cannot be performed under the root directory by make, but requires access to the directory of the sdk Makefile file.

Copy link
Member

Choose a reason for hiding this comment

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

In #4323, c sdk adds some third-party libraries using submodules. git doesn't clone or use submodules by default. Needs to make or scan them after adding them.


#4323 中,c sdk使用了submodule的方式添加了一些第三方库。git默认是不clone或使用submodule的,需要添加它们之后再make或扫描。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

github/codeql#14703
I raised this issue to the codeql community. We can follow it and reply under this issue.


我向codeql 社区提了这个问题的issue,大家可以关注并在这个issue下面回复一下

Copy link

Choose a reason for hiding this comment

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

The matrix does not define a language property, so matrix.language evaluates to null.

@@ -33,6 +33,7 @@ jobs:
matrix:
os: [ ubuntu-latest, macOS-latest ]
java: [ 8, 11 ]
language: ['java', 'go', 'c']
Copy link
Member

@pandaapo pandaapo Nov 7, 2023

Choose a reason for hiding this comment

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

Does the C/C++ language name here need to be consistent with the cpp in the following if: matrix. language=='cpp' || matrix.language == 'csharp'?

Copy link

Choose a reason for hiding this comment

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

yes

@pandaapo
Copy link
Member

pandaapo commented Nov 7, 2023

Now Build C can be executed. And the error is reported: make: * * * No targets specified and no makefile found. Stop. Error: Process completed with exit code 2. As stated in this comment.

To solve this error, I tried the command make -C ./eventmesh-sdks/eventmesh-sdk-c before, and the following error will be reported:

make: Entering directory '/home/runner/work/eventmesh/eventmesh/eventmesh-sdks/eventmesh-sdk-c'

Building  ==> third_party/libcurl.a...
/bin/sh: 1: ./configure: not found
make: *** [makefile:56: third_party/libcurl.a] Error 127
make: Leaving directory '/home/runner/work/eventmesh/eventmesh/eventmesh-sdks/eventmesh-sdk-c'
Error: Process completed with exit code 2.

Based on this comment, I am not sure if adding some commands will work, such as changing the script to the following:

git submodule init
git submodule update 
make -C ./eventmesh-sdks/eventmesh-sdk-c

@xwm1992
Copy link
Contributor Author

xwm1992 commented Nov 7, 2023

Now Build C can be executed. And the error is reported: make: * * * No targets specified and no makefile found. Stop. Error: Process completed with exit code 2. As stated in this comment.

To solve this error, I tried the command make -C ./eventmesh-sdks/eventmesh-sdk-c before, and the following error will be reported:

make: Entering directory '/home/runner/work/eventmesh/eventmesh/eventmesh-sdks/eventmesh-sdk-c'

Building  ==> third_party/libcurl.a...
/bin/sh: 1: ./configure: not found
make: *** [makefile:56: third_party/libcurl.a] Error 127
make: Leaving directory '/home/runner/work/eventmesh/eventmesh/eventmesh-sdks/eventmesh-sdk-c'
Error: Process completed with exit code 2.

Based on this comment, I am not sure if adding some commands will work, such as changing the script to the following:

git submodule init
git submodule update 
make -C ./eventmesh-sdks/eventmesh-sdk-c

#4542
as you mentioned the error, maybe merge this pull request can fix this error.

@pandaapo
Copy link
Member

pandaapo commented Nov 8, 2023

From the error information reported, it seems that the third-party libraries as git submodules have not been introduced yet, as I found the file 'configure.ac' in one of the third-party libraries 'curl'.

Building  ==> third_party/libcurl.a...
autoreconf: error: 'configure.ac' is required
make: *** [third_party/libcurl.a] Error 1

@xwm1992
Copy link
Contributor Author

xwm1992 commented Nov 8, 2023

企业微信截图_2838c2da-7988-444c-aa6b-4d7bcb1432ba

Since compile c code face the git permission error, I'll jump the c code compile and codeql check for now.

@xwm1992 xwm1992 merged commit 3a580dc into apache:master Nov 9, 2023
@aibaars
Copy link

aibaars commented Nov 9, 2023

Since compile c code face the git permission error, I'll jump the c code compile and codeql check for now.

Have you tried submodules: true on the actions/checkout step? It converts SSH URLs to HTTPS ones, which should avoid the public key permission errors.

# Whether to checkout submodules: true to checkout submodules or recursive to
# recursively checkout submodules.
#
# When the ssh-key input is not provided, SSH URLs beginning with
# [email protected]: are converted to HTTPS.
#
# Default: false
submodules: ''

@pandaapo
Copy link
Member

tried submodules: true on the actions/checkout step

It's helpful.

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.

6 participants