You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/09/12 21:39:32 UTC
[GitHub] [incubator-nuttx] v01d opened a new pull request #1771: Make CI fail on first error and not have to finish the job to fail
v01d opened a new pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #1771: Fix -x of testbuild.sh; Do not use -x on CI to have all build errors
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#discussion_r487555201
##########
File path: .github/workflows/build.yml
##########
@@ -161,7 +161,7 @@ jobs:
export CCACHE_DIR=`pwd`/ccache
mkdir $CCACHE_DIR
cd sources/testing
- ./cibuild.sh -c -x testlist/${{matrix.boards}}.dat
+ ./cibuild.sh -c testlist/${{matrix.boards}}.dat
Review comment:
Yeah three things that will help with this:
1) Moving to GitHub Container Registry from packages (that ticket I shared). This will mean we get rid of some of the extra logic in the workflow.
2) GitHub has a planned feature to allow sharing workflows between repos.
3) We can move more of the shared logic into a single GitHub action. This is going to be worth it longer term, just requires motivation to build the action in NodeJS which I have not been excited about. Maybe I can set a goal for myself to have that in the release after the current one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis edited a comment on pull request #1771: Make CI fail on first error and not have to finish the job to fail
Posted by GitBox <gi...@apache.org>.
acassis edited a comment on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691658268
I think it could behave this way: 1) if it is a coding style error or a warning that was promoted to error, then the building system should carry on. 2) if it is a real compilation error then it could stop to save processing power and let others PR to finish faster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #1771: Fix -x of testbuild.sh; Do not use -x on CI to have all build errors
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#discussion_r487552317
##########
File path: .github/workflows/build.yml
##########
@@ -161,7 +161,7 @@ jobs:
export CCACHE_DIR=`pwd`/ccache
mkdir $CCACHE_DIR
cd sources/testing
- ./cibuild.sh -c -x testlist/${{matrix.boards}}.dat
+ ./cibuild.sh -c testlist/${{matrix.boards}}.dat
Review comment:
Will do.
It is a real pain to maintain this in two places.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1771: Fix -x of testbuild.sh; Do not use -x on CI to have all build errors
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691695640
Ok, I removed the -x and updated the PR description/commit msg.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1771: Fix -x of testbuild.sh; Do not use -x on CI to have all build errors
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#discussion_r487552052
##########
File path: .github/workflows/build.yml
##########
@@ -161,7 +161,7 @@ jobs:
export CCACHE_DIR=`pwd`/ccache
mkdir $CCACHE_DIR
cd sources/testing
- ./cibuild.sh -c -x testlist/${{matrix.boards}}.dat
+ ./cibuild.sh -c testlist/${{matrix.boards}}.dat
Review comment:
Please don't forget modify apps and testing git too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] liuguo09 commented on pull request #1771: Fix -x of testbuild.sh; Do not use -x on CI to have all build errors
Posted by GitBox <gi...@apache.org>.
liuguo09 commented on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691746971
@btashton @v01d Yes, it's intentional to cover all possible failures at first time to ease development, as @xiaoxiang781216 shows more info in comments.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1771: Make CI fail on first error and not have to finish the job to fail
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691669115
Sometime, the change may introduce the break in the multiple locations. If we terminate the build when the first error hit, we have to fix the error and try the result one by one which make the recovery process become much longer and painful, then more upcomming PR will be blocked for long time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1771: Make CI fail on first error and not have to finish the job to fail
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691681045
I understand the usefulness of having this continue to run, I'm not opposed.
However, do note that what I'm actually fixing here is the behavior of `-x` parameter, which is actually used on CI. Maybe the -x is to have the script abort on other errors, but with `-x` I would expect any build failure to abort the script. In other words, maybe the path is merge this PR and then remove `-x` from the workflow.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1771: Make CI fail on first error and not have to finish the job to fail
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691669115
Sometime, the change may introduce the break in the multiple locations. If we terminate the build when the first error hit, we have to fix the error one by one which make the recovery process become much longer and painful, then more PR will be blocked for long time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton merged pull request #1771: Fix -x of testbuild.sh; Do not use -x on CI to have all build errors
Posted by GitBox <gi...@apache.org>.
btashton merged pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1771: Make CI fail on first error and not have to finish the job to fail
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691683967
> I understand the usefulness of having this continue to run, I'm not opposed.
> However, do note that what I'm actually fixing here is the behavior of `-x` parameter, which is actually used on CI. Maybe the -x is to have the script abort on other errors, but with `-x` I would expect any build failure to abort the script. In other words, maybe the path is merge this PR and then remove `-x` from the workflow.
Yes, it will be good that we remove -x from the workflow together with this path.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #1771: Make CI fail on first error and not have to finish the job to fail
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691658268
I think it could behave this way: 1) if it is a coding style error or a warning was promoted to error, then the building system should carry on. 2) if it is a real compilation error then it could stop to save processing time and let others PR to finish faster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #1771: Fix -x of testbuild.sh; Do not use -x on CI to have all build errors
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#discussion_r487555201
##########
File path: .github/workflows/build.yml
##########
@@ -161,7 +161,7 @@ jobs:
export CCACHE_DIR=`pwd`/ccache
mkdir $CCACHE_DIR
cd sources/testing
- ./cibuild.sh -c -x testlist/${{matrix.boards}}.dat
+ ./cibuild.sh -c testlist/${{matrix.boards}}.dat
Review comment:
Yeah two things that will help with this:
1) Moving to GitHub Container Registry from packages (that ticket I shared). This will mean we get rid of some of the extra logic in the workflow.
2) GitHub has a planned feature to allow sharing workflows between repos.
3) We can move more of the shared logic into a single GitHub action. This is going to be worth it longer term, just requires motivation to build the action in NodeJS which I have not been excited about. Maybe I can set a goal for myself to have that in the release after the current one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #1771: Make CI fail on first error and not have to finish the job to fail
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691543083
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1771: Make CI fail on first error and not have to finish the job to fail
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691669115
Sometime, the change may introduce the break in the multiple locations. If we terminate the build when the first error hit, we have to fix the error and try the result one by one which make the recovery process become much longer and painful, then more upcomming PR will be blocked to pass the check for long time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #1771: Fix -x of testbuild.sh; Do not use -x on CI to have all build errors
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#discussion_r487556736
##########
File path: .github/workflows/build.yml
##########
@@ -161,7 +161,7 @@ jobs:
export CCACHE_DIR=`pwd`/ccache
mkdir $CCACHE_DIR
cd sources/testing
- ./cibuild.sh -c -x testlist/${{matrix.boards}}.dat
+ ./cibuild.sh -c testlist/${{matrix.boards}}.dat
Review comment:
> 2. GitHub has a planned feature to allow sharing workflows between repos.
That's key
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1771: Fix -x of testbuild.sh; Do not use -x on CI to have all build errors
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691696202
LGTM, @btashton please merge it if you don't have the furture concern.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1771: Make CI fail on first error and not have to finish the job to fail
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691694473
> > I understand the usefulness of having this continue to run, I'm not opposed.
> > However, do note that what I'm actually fixing here is the behavior of `-x` parameter, which is actually used on CI. Maybe the -x is to have the script abort on other errors, but with `-x` I would expect any build failure to abort the script. In other words, maybe the path is merge this PR and then remove `-x` from the workflow.
>
> Yes, it will be good that we remove -x from the workflow together with this path.
@btashton sounds also OK to you?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #1771: Make CI fail on first error and not have to finish the job to fail
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691543337
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #1771: Make CI fail on first error and not have to finish the job to fail
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691669115
Sometime, the change may introduce the break in the multiple locations. If we terminate the build when the first error hit, we have to fix the error and try the result one by one which make the recovery process become much longer and painful, then more upcomming PR will be blocked to pass the check for long time. On the other hand, since github allocate a fixed number of VM(20) for each PR indepently, other PR can't finish faster even we terminate the failed job as soon as possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on pull request #1771: Make CI fail on first error and not have to finish the job to fail
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #1771:
URL: https://github.com/apache/incubator-nuttx/pull/1771#issuecomment-691695015
Sounds good to me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org