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/03/07 13:45:19 UTC
[GitHub] [incubator-nuttx] johannes-nivus opened a new pull request #471:
Changes for Freedom K28 USB device support
johannes-nivus opened a new pull request #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #471: Changes
for Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596163161
> @xiaoxiang781216
>
> > I can't understand why another repo can resolve nxstyle update issue.
>
> Assume The tool in master of nuttx is changing.
>
> What workflow can you use to get the latest version of the tool?
>
> 1. Reabse your work on master before submission. - With the instability in master this is risky.
Why I can't do this?
git branch --track upstream origin/master
git checkout upstream
git cherry-pick mywork
./toos/checkpatch.sh -c upstream
git push ....
since the big work always done in a feature branch, I don't think there is any risky to rebase the feature branch against master.
> Your good work WILL be impeded by the lack of QA on master.
Actually, contributor must rebase/cherry-pick his patch against master before send PR.
> 2. stash your work, fetch and build nxstyle from master at HEAD. Copy it do /usr/local/bin
> stash pop and run the tool.
> 3. Have a submodule with the tools in it.
> cd tools;git fetch nuttx-tools;git reset hard nuttx-tools/master;cd ..
> Work as normal.
But I need remember update/compile/copy another repo every time:(.
>
> Then CI only uses nuttx-tools
As you said nutx-tools just for CI, it's hard to tell people to clone this repo. I still think the nuttx/tools is a better place to put the tool which can be used by developer machine directly. nuttx-tools is for the script that need run on github/apache server.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #471: Changes
for Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596090500
@johannes-nivus Please fix the warning reported by nxstyle and update PR again, thanks.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #471: Changes for
Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596200555
Yes, there are many issues with nxstyle that need to be fixed. Most are listed in the Issues list. You have to know when to ignore the complaint and when not to. Unfortunately, people with less familiarity with the coding style cannot usually distinguish what is a true coding standard problem and what is a bug in the tool.
nxstyle does have an issue with comments to the right of lines. This is a documented Issue. You must ignore those complaints. You must not modify the code to match the errors in the tool.
nxstyle will be fixed someday. The work-around for now is to check the issues and ignore erroneous reports.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo merged pull request #471: Changes for
Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus commented on issue #471: Changes
for Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596112057
@xiaoxiang781216 @patacongo I'm not quite sure what to do with kinetis_k28memorymap.h.
I fixed the missing blank line and the invalid sections.
But the other errors are related to the multiline comments in the end of lines.
Should I convert them to single line comments above or below the according line?
This would require additional blank lines...
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] davids5 commented on issue #471: Changes for
Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596126367
@johannes-nivus - extend the comment lines.
This is use as a ruler. Once they are all the same length and long enough the log line errors will go away.
see this PR https://github.com/apache/incubator-nuttx/pull/470 for an example.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus commented on issue #471: Changes
for Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596091296
@xiaoxiang781216 Hmmm, there are much more warnings than changes in the initial commit...
Nevertheless I will take a look.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus edited a comment on issue #471:
Changes for Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
johannes-nivus edited a comment on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596128340
@patacongo Why did you merge?
I haven't yet commited the style fixes for the obvious errors, because I'm still a little struggling with git and github. Should I create another PR? Or will you revert and I commit to this PR?
Nevermind. I will file another PR, because deleting my branch will also fix my current git issues.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] davids5 commented on issue #471: Changes for
Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596095109
@johannes-nivus - as @xiaoxiang781216 asked, but first rebase on current master. The style tool is unfortunately changing too often and not in a submudule.
@liuguo09 @xiaoxiang781216 Please confirm nxstyle is always coming from current master, or move the tool to a seprate repo as was suggested in the past to avoid this nonsense.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus commented on issue #471: Changes
for Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596197098
@patacongo I had a look at the issues with kinetis_k28memorymap.h and nxstyle.
The problem is, that preprocessor lines aren't checked for comments right of code. That gives many errors for the right of code comments spanning multiple lines, despite they are acceptable in the coding standard.
There are at least 2 1/2 possibilities:
1.: fix nxstyle (I had a look already but since pp processing is early and uses continue thats not so easy.
2.: discourage usage of multiline comments right of code for preproccesor
2 1/2: discourage usage of multiline comments right of code at all
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus commented on issue #471: Changes
for Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596128340
@patacongo Why did you merge?
I haven't yet commited the style fixes for the obvious errors, because I'm still a little struggling with git and github. Should I create another PR? Or will you revert and I commit to this PR?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #471: Changes
for Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596101489
> @johannes-nivus - as @xiaoxiang781216 asked, but first rebase on current master. The style tool is unfortunately changing too often and not in a submudule.
>
> @liuguo09 @xiaoxiang781216 Please confirm nxstyle is always coming from current master,
Yes, the nxstyle tool come from the master, here is the invocation pointer:
https://github.com/apache/incubator-nuttx/blob/master/.github/workflows/main.yml#L44
> or move the tool to a seprate repo as was suggested in the past to avoid this nonsense.
I can't understand why another repo can resolve nxstyle update issue.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] davids5 commented on issue #471: Changes for
Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596127405
@xiaoxiang781216
> I can't understand why another repo can resolve nxstyle update issue.
Assume The tool in master of nuttx is changing.
What workflow can you use to get the latest version of the tool?
1) Reabse your work on master before submission. - With the instability in master this is risky.
Your good work WILL be impeded by the lack of QA on master.
2) stash your work, fetch and build nxstyle from master at HEAD. Copy it do /usr/local/bin
stash pop and run the tool.
3) Have a submodule with the tools in it.
cd tools;git fetch nuttx-tools;git reset hard nuttx-tools/master;cd ..
Work as normal.
Then CI only uses nuttx-tools
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #471: Changes for
Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596200729
> @patacongo Why did you merge?
I didn't see any ligitimate coding style problems. There was nothing to be fixed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] davids5 commented on issue #471: Changes for
Freedom K28 USB device support
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #471: Changes for Freedom K28 USB device support
URL: https://github.com/apache/incubator-nuttx/pull/471#issuecomment-596189556
> Why I can't do this?
> git branch --track upstream origin/master
> git checkout upstream
> git cherry-pick mywork
> ./toos/checkpatch.sh -c upstream
> git push ....
You can. But I would ask what is the value of all the commands you are proposing over just using `git rebase master`?
Also consider what will happen. You will need to fix the CS issues and commit the changes here not in mywork. If you care you will rebase and squash the noise. Then drop the commits on mywork cherry-pick back. That seams like a lot of busy work.
>Actually, contributor must rebase/cherry-pick his patch against master before send PR.
Yes. But it is not until it they have to take an update from upstream master that they have to debug the issues that came in while they were working.
I will go out on a limb and guess that xiaomi has it's own stable branch and works on that.
I makes sense given the lack of QA on master. I will bet most all downstream have to take this defensive stance.
Our goal and responsibility to the project should be: Master always builds and PR's are tested before merging. This is hard to do with the project as it is. But thanks to @liuguo09, you and all the people that worked on the CI we are getting there!
> But I need remember update/compile/copy another repo every time:(.
Remember the discussion of `make check_format` you shot down?
check_format:
check and pull the host-tools repo if need be
build the tool
run the command
Now you will have `mywork` formatted with the right version of the style tool. You can begin the nxstyle dance, squashing and have what you needed were you needed it.
>As you said nutx-tools just for CI, it's hard to tell people to clone this repo. I still think the nuttx/tools is a better place to put the tool which can be used by developer machine directly. nuttx-tools is for the script that need run on github/apache server.
Sorry I should have called it host-tools. The directory in nuttx/tools should be a repo host-tools.
Well I am not going to revisit the submodule augment with you, other then to say this uses case is exactly why components are needed. When a project that uses git needs components, submodules are a native option.
----------------------------------------------------------------
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
With regards,
Apache Git Services