Use Travis CI for automated testing #1
Conversation
|
Hi @per1234, Thanks for opening this pull request! I have a few things I think we should discuss as a team before we propagate to more libraries:
If the plan is to enforce it, we could also consider a git precheck hook.
|
@lxrobotics and I set up the code style check in the ArduinoIoTCloud and ArduinoCloudThing repositories a month ago as somewhat of a trial and demonstration, then @lxrobotics made a proposal in the #core Slack channel that this be expanded to the rest of the library repositories. A week later, there were no objections and some nice "thumbs up", so I took that as an implicit approval to proceed. I know none of that counts as a formal agreement. I think this is something that's really important to put in place. I also understand that it's a pretty significant change.
My opinion is that the same code style should be used consistently on all code that runs on microcontrollers (and perhaps even beyond that scope). I don't care what that style looks like; I just want there to be a standard. https://raw.githubusercontent.com/arduino/Arduino/master/build/shared/examples_formatter.conf is the closest thing Arduino has to an existing standard, though I understand it was not necessarily created with my usage in mind.
The advantage of that is avoiding cluttering up all the repositories with an extra file (and usually folder). The disadvantage is that when you add a word to the ignore list to fix a false positive, you also increase the changes of a false negative. That word was on the list of commonly misspelled words for a reason. Sharing a common ignore list causes each ignored word to have a much larger scope than necessary. A shared ignore list also means that someone whose PR introduces a false positive will need to submit a separate PR to the shared list repo and those two PRs will need to be coordinated (no need to merge the PR to the ignore words list if the PR that requires it is going to be rejected). |
|
Hi @sandeepmistry, As @per1234 mentioned there was my proposal within the core channel on Slack on 7th of May. As of today no one has objected to the proposal (and quite a few people have thumbed it up). I'd consider this an implicit agreement. However, if you feel that further discussion is necessary than by all means let's have one - on Slack which would be im my opinion the better medium for this discussion. |
Travis CI will compile each of the example sketches MKR Zero using the latest release of arduino-cli every time someone commits to the repository or submits a pull request.
TravisBuddy automatically comments on any pull request that results in a failed Travis CI build. This will cause the author of the pull request to get a notification of the failure and asking them to check the build results to see if they can fix the problem. The hope is that this will result in more pull requests that are valid before a maintainer needs to get involved. It also makes it easier on the pull request author because they don't need to remember to come back to the repository to check the CI build result.
The Artistic Style formatter tool was used with the https://raw.githubusercontent.com/arduino/Arduino/master/build/shared/examples_formatter.conf configuration to format all code in the repository. I did not touch the files in src/minmea because this appears to be an external resource.
Use the Artistic Style formatting tool (also used for the Arduino IDE's auto format feature) to check all code files in the repository for compliance with the Arduino code formatting style. I excluded src/minmea from the check because this appears to be an external resource.
Use codespell to check for commonly misspelled words during the Travis CI build. In the event of false positives, the words can be added to extras/codespell-ignore-words-list.txt, after which codespell will ignore those words. I excluded src/minmea from the check because this appears to be an external resource.
|
Some more thoughts on this:
So overall, it's a good direction to go in, but we should take it slow, and also I think we have higher priority things to work on at the moment. cc/ @pnndra |
my understanding is that a code style was already in place based on what i heard from simone/cristian. if it isn't then we need to have it anyway.
i was proposing to have an initial period in which we just issue warnings, if possible, and enforce rules only after this grace period. of course we should have a document that explains how to run the code through a checker before committing, in order to prevent wasting time on files with formatting issues.
i think that, if possible, we should not include the file but rather reference it from a central git repo through a submodule. this would allow the file to propagate automatically everywhere. in terms of who is going to be responsible for this i think that at the moment alex and per are perfect candidates for this for the time being. as they will become busier and we'll have more people on board things may change.
having consistent code and documentation is really a priority as it is a metric for the quality of our products. also, this is sort of a starting test for introducing continuous integration more in general which also will be a crucial point to avoid regression |
|
Hi Dario, this is what I was referring to: https://www.arduino.cc/en/Reference/APIStyleGuide It was written by the Founder Tom Igoe and it is one of the pillars of Arduino. It goes together with this other guide: https://www.arduino.cc/en/Reference/StyleGuide These two documents and many others that give a clear picture on how things should be done can be found here: https://www.arduino.cc/en/Hacking/HomePage The specific link https://www.arduino.cc/en/Hacking/LibraryTutorial will let you to the above mentioned links and provides some information on how we ask our contributors to write the libraries. I hope this gives some more clues on how things should be done to keep the essence of Arduino consistent throughout the new libraries and documentation. Simone |
I can easily use Travis CI's
I'm happy to help with that. The Arduino IDE's Auto Format can easily be customized. I know how to integrate Artistic Style into Notepad++. I can provide an example of an astyle command to run from the command line or script to do the formatting on a project. I don't have any experience with setting up other editors/IDEs for auto formatting with Artistic Style, but I can investigate that if necessary. I'm also working on a pre-commit Git hook for checking formatting compliance right now.
By "the file", do you mean a script that contains a code formatting check function (and perhaps other generally useful functions)? That file could be downloaded or cloned from its dedicated repository during the CI build so a submodule wouldn't necessarily be needed.
I'm happy to help out in any way I am able. I really enjoy working on this sort of thing and I have a good amount of experience using Travis CI. Unfortunately, not much of that can be checked for automatically so it doesn't provide any guidance for this project. The only thing in that guide I could see possibly being checked automatically is camel case names, but Artistic Style doesn't do that. As I mentioned in arduino/ArduinoCore-avr#71 (comment), in addition to the Artistic Style configuration which defines the code formatting style, I'd like to see Arduino write a more detailed code style guide that contains additional guidelines. I think the ArduinoIoTCloud and ArduinoCloudThing repositories can be used as a case study on this proposal. I'm not involved with the development work in those repositories, but from what I've seen from watching those repos it seems like it has been of benefit. I do think providing some documentation on how the developers can easily make their code comply with the standard would have been helpful. |
|
Thanks for the detailed response Simone. i will link these in the FT home page for future reference. |
|
@per1234 by the file i mean for example the travis files you committed in this repo. as @sandeepmistry mentioned it doesn't make sense to have the same things everywhere as if we need to change it we'll need to traverse many different repos. |
|
The files I added in this PR are Regarding Regarding I think it would be quite difficult to use a submodule for |
|
Well, I'm glad we've got the discussion going 😃 I agree with @per1234 that it does not make sense to have a global The astyle configuration file examples_formatter.conf we are using within this PR (and also within Furthermore I'd like to point out that the Arduino IDE itself uses formatter.conf for its auto-format functionality which is a slightly less restrictive version of examples_formatter.conf. With the Arduino libraries/examples setting the example for users looking to extend and contribute I consider it a good idea to subject ourselves to a higher standard. As a last point I want to add that |
|
thanks for the clarifications. once we have this nailed up i would suggest to add some of this detail to confluence so that everyone knows how to set up properly repositories. Dario |
|
Here's an example of why it's important for Arduino to use an official code style standard and have an automated check for compliance: Any conscientious contributor will want to follow the existing code style of the file they're working in. They would waste a lot of time trying to determine what the prevailing style is in that file and eventually give up and pick something at random. We currently have a PR where the contributor added some functions to the file and decided on two space indents: |
|
I wrote a script for the code formatting check that can be shared between all of Arduino's CI configurations: I prepared a demonstration of it in use in a Travis CI configuration here: Regarding the codespell ignore words list, I have changed to specifying this directly in the codespell command as a comma-separated list using the I wrote Git hooks for both the code formatting check and codespell here: I welcome any feedback on these proposed solutions to the concerns regarding this PR. |
|
There have been no objections to the solution I proposed in my last reply after over a month so I'd like to try to move forward with this. To do that, I'll need someone to create a repository for the CI helper script. For my demonstration, I named this repository "ci-resources" with the idea that it might later be used to contain other resources for CI builds to share, but you're welcome to pick any name you like. I think the arduino organization would be the best place for the repository, since the contents of the repository might well be used for more than only testing of Arduino libraries in time. Once that repository is created, I can submit a pull request to add my script. |
|
Please note my feedback above (#1 (comment), #1 (comment)) has not fully been addressed yet. |
|
I guess you mean this:
and this:
I haven't seen any disagreement to it in the 3.5 months since I implemented it in the ArduinoIoTCloud and ArduinoCloudThing repositories, the 2.5 months since Alexander proposed it on Slack, or the >2 months since I opened this PR. Earnest efforts have been made to bring it to the attention of interested parties and they have been given plenty of time to comment. I feel that in the real world this is as close as it gets to the formal agreement you want. We have an existing well defined code style that has been in use for Arduino sketches for years and works perfectly fine. Why not use the one we already have and move on to more important things than talking about how many spaces are in an indent? It makes no sense to me for Arduino to have one code style for sketches and another for libraries. Changing the code style in the sketches would be disruptive to the users and require a lot of work to update documentation. It would cause a style mismatch with the many existing tutorials that use Arduino examples but are not in Arduino's control to update. Using the established sketch code style in library code just seems like the logical way to proceed. |
|
My 2 cents: |
|
So there are a few things that are unresolved: Should existing code be formatted?The only statement specifically against doing this so far is from @facchinm, though @sandeepmistry's comment "We could also possibly extract the code style from existing libraries and cores in order to minimize changes." is related. I disagree with the argument that it "makes git blame useless". It doesn't make it useless, it only makes it slightly less convenient due to introducing a commit that doesn't have any functional effect on the code. You can still go back one commit in the history to get the blame previous to the formatting commit. In a previous discussion about formatting the Arduino AVR Boards code, the concern was raised that it will cause merge conflicts in pull requests. (arduino/ArduinoCore-avr#71 (comment)) Even if the decision is made to not format or enforce a code style on existing code, it is still worthwhile to establish an official Arduino code style to use on all future code. What should the official Arduino code style be?There has been no specific disagreement to using the code style defined in examples_formatter.conf so far. @sandeepmistry's statement "We could also possibly extract the code style from existing libraries and cores in order to minimize changes." could be considered a counter proposal. How can a formal agreement be reached on the official Arduino code style?Would we need to make a list of interested parties and wait for all of them to make a statement in favor of the chosen code style before it's considered a formal agreement? I appreciate the democratic approach, but I also think there's a time for executive decisions when running a company. The organizational chart indicates that @pnndra is the person who has final say in this. |
|
here i am with some guidance. after some discussion with @mbanzi @facchinm and @sandeepmistry there seems to be consensus on a number of things:
having said this i would request @per1234 to summarize in a spreadsheet the state for all our libraries listing the number of commits, branches, forks and open PRs. while doing this it would make sense to summarize the conding style for each repo, possibly categorizing it so that we can later look at the spreadsheet and weight our choice based on the most used style weighted by library popularity (commits, PRs, etc). once we have this job done (please @per1234 let me have a time estimate for this), we can set up the style rules and start enforcing them on all "new" libraries, with small number of commits and PRs. regarding more popular libraries we can create a new master branch and set it as default while keeping the old master branch which will keep the PRs attached. this will allow transitioning to the clean files without losing the history and without upsetting everyone. of course we'll be encouraging community to migrate PRs to the new master so that they can be merged. i apologize with @cmaglie for not consulting him but hope he agrees with this approach and would like to see this topic closed asap as hopefully we'll be engaging more and more with community and the earliest we fix this, the less work we'll have to do to amend work done. |
|
I'm approximately half finished with the libraries spreadsheet. I estimate I will finish it tomorrow. |
|
Here's the libraries spreadsheet: I tried to skip over the files of any external libraries bundled with the Arduino libraries, as it probably is best to only modify those files upstream. In some cases it was difficult to determine what was and wasn't to be considered an external library. In many cases, the coding style was not consistent throughout each library's code. I indicated this by adding "(mixed)" to the cell. In these cases, I estimated what was the predominant style of the library. Unweighted results for the predominant styles:Indent
Braces attached to function definition
Braces attached to statement
weighted (by commits, stars, forks, and PRs) results for the predominant styles:Indent
Braces attached to function definition
Braces attached to statement
Percentages don't sum to 100% due to the existence of other indentation styles, libraries where I couldn't decipher a predominant style, or lack of data. I didn't include the bundled libraries in the weighted results because I didn't know how to do the weighting for them. They are included in the unweighted results. Note that with the exception of "Braces attached to function definition", these match the examples_formatter.conf code style. |
|
I created a script with functions for the common actions used for compilation testing and added it to the same repository that contains the code formatting script. The idea is to reduce code duplication by sharing these functions between all CI configurations: I've updated my demonstration .travis.yml file to use them as well as the shared code formatting check script: Any contributions to improve those functions or suggestions for additional functions to add are welcome. |
|
Thank you very much for preparing a collection of scripts to be reused across all repositories. In my opinion there is all the functionality present which we use within the regular CI build but either rewrite it manually or copy from project to project. Good work 👍 |
|
Closing, shall be replaced by GitHub Action based CI (#10). |
per1234
added
type: enhancement
Travis CI is used to run the following tests on the repository after every commit or pull request:
arduino-cli.The file
extras/codespell-ignore-words-list.txtis used in case of false positives during the check for commonly misspelled words (all words in the file are ignored by codespell).