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

Changing return type of vtimer_set_msg #2545

Merged
merged 1 commit into from
Mar 19, 2015

Conversation

kushalsingh007
Copy link
Member

-The current return value is not used anywhere and has no meaning attached to it.

@miri64
Copy link
Member

miri64 commented Mar 5, 2015

I don't see why you need to include your Commit from #2544

@kushalsingh007
Copy link
Member Author

@authmillenon Sorry , Commit from #2544 got added by accident ( they were part of branch I guess ) , apart from that is it alright ?

@kushalsingh007
Copy link
Member Author

@authmillenon just the last one , what is the reason for build failing for this one ( I guess it's because the unix script is also a part of it ). Can you help me with understanding the output of Travis (in general) , it will be really helpful to understand the problems myself. From what I see there is a list of different platforms for which it is built and some static tests.
Do you have any links or references that I could have a look at :) Thanks got your help in advance

@jnohlgard
Copy link
Member

@kushalsingh007 At the bottom of the comments thread you'll find the text "Failed — The Travis CI build failed ", click on the link "Details" to the right of that text.

(I restarted your build just now so it might still say "Waiting to hear about e916630 — The Travis CI build is in progress ", you can watch the build live on the Details page)

The new page displays the "build matrix". Any red items are failed, green are fine, yellow are running, and sometimes they can be grey, the grey is usually that the Travis server was overloaded and the build was killed for taking too long to complete.

If the static-tests group is red, that's the first one to look at. Click on the job number (6877.1) to show the log. It will display the following:

.... snip ....
$ ./dist/tools/travis-scripts/build_and_test.sh

First, rewinding head to replay your work on top of it...

Applying: Adding a script to check for line endings (valid UNIX ending)

Applying: Changing the return type of vtimer_set_msg to void

ERROR: This change introduces new whitespace errors

The command "./dist/tools/travis-scripts/build_and_test.sh" exited with 1.

Done. Your build exited with 1.

Look for error lines, in your case it says there are whitespace errors.

(For whitespace errors specifically, the easiest way to find them is to run git diff --check master in your local repo to see if which files cause whitespace errors.)

Next, go back to the build matrix, click on the next failing build, read the text.

If there are some boards failing on some tests it may be because of a real error, or may be because of Travis' general flakyness... The easiest way to find out is to try and build for that board on your PC and see if you get any errors or warnings. If it works on your PC, then all we have to do is restart the job a couple of times until Travis is happy. Some days Travis is fine right away, other days it may take several attempts until it works (nobody is happy with this method, but it's better than no CI at all)

@jnohlgard
Copy link
Member

In your particular case, Travis is not failing because of flakiness but because of real errors in your code. This is good because this is why we have a CI system.

https://travis-ci.org/RIOT-OS/RIOT/jobs/53258558

"make" -C /home/travis/build/RIOT-OS/RIOT/sys/vtimer

/home/travis/build/RIOT-OS/RIOT/sys/vtimer/vtimer.c:395:6: error: conflicting types for ‘vtimer_set_msg’

void vtimer_set_msg(vtimer_t *t, timex_t interval, kernel_pid_t pid, uint16_t type, void *ptr)

^

In file included from /home/travis/build/RIOT-OS/RIOT/sys/vtimer/vtimer.c:33:0:

/home/travis/build/RIOT-OS/RIOT/sys/include/vtimer.h:111:5: note: previous declaration of ‘vtimer_set_msg’ was here

int vtimer_set_msg(vtimer_t *t, timex_t interval, kernel_pid_t pid, uint16_t type, void *ptr);

^

@PeterKietzmann
Copy link
Member

@gebart thanks for the detailed description!
@kushalsingh007 when changing the vtimer_set_msg function you also need to adapt the prototype in RIOT/sys/include/vtimer.h
@ALL aren't the return types in vtimer_init and vtimer_remove also a bit useless?

@kushalsingh007
Copy link
Member Author

I think not only do vtimer_init and vtimer_remove have useless return values , but set_longterm and update_shortterm too.

@PeterKietzmann
Copy link
Member

In addition: I was thinking more about an extension of the return values than changing to void. @kushalsingh007 are you going to take the line-endings-check commit out?

@kushalsingh007
Copy link
Member Author

@PeterKietzmann I'll do it ( removing the line-ending-check commit ) as soon as I get back to my computer :) , can you tell me what you mean by extension of return values ?

@gebart : Thanks for the great explanation

@PeterKietzmann
Copy link
Member

I just meant that "just" returning 0 at the end of a function could be "extended" by a bit more error handling. However, this would result in a more or less hard API-change that is maybe also not wanted at the moment. So removing the unrelated commit and also adapting the prototype should be fine for now.

@kushalsingh007 kushalsingh007 force-pushed the refactor_vtimer_set_msg branch 4 times, most recently from b2272ef to dddeb84 Compare March 6, 2015 21:37
@kushalsingh007
Copy link
Member Author

The error is due to the build getting stalled .
@PeterKietzmann removed the reference to unix-line-ending and changed the return type of vtimer_init and vtimer_remove too . I think the problem problem has been solved now.

@kushalsingh007
Copy link
Member Author

Can this PR be merged now ?

@miri64 miri64 added Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Mar 8, 2015
@kushalsingh007
Copy link
Member Author

Does this commit have any more problems left ?
Edit : Review needed

@OlegHahm
Copy link
Member

Can you change the first line of the commit message to something like
vtimer: changing return type of several functions to void ?

@OlegHahm
Copy link
Member

Doxygen needs adaption, too.

@kushalsingh007 kushalsingh007 force-pushed the refactor_vtimer_set_msg branch from dddeb84 to dbfb69f Compare March 18, 2015 16:36
@kushalsingh007
Copy link
Member Author

@OlegHahm : I have changed the commit message but I am not sure I follow you about doxygen adaptation , isn't the documentation auto generated ?

@OlegHahm
Copy link
Member

Yes, but it is generated by doxygen based on the comments in doxygen style.

Thus, you need to remove the @returns lines from the headers.

@kushalsingh007 kushalsingh007 force-pushed the refactor_vtimer_set_msg branch from dbfb69f to 9456b25 Compare March 18, 2015 17:24
@kushalsingh007
Copy link
Member Author

Done

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 18, 2015
@OlegHahm
Copy link
Member

Looks good. Please squash.

@kushalsingh007 kushalsingh007 force-pushed the refactor_vtimer_set_msg branch from 9456b25 to f93af1e Compare March 18, 2015 21:41
@kushalsingh007
Copy link
Member Author

Done squashing :)

@OlegHahm OlegHahm removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Mar 18, 2015
@OlegHahm
Copy link
Member

You could add to the commit message which functions were affected.

- Changed the return type for vtimer_init,vtimer_set_msg and vtimer_remove.
- Current return value is not used anywhere and has no meaning attached to it.
- Fix for RIOT-OS#1533
- Removed tests which checked for vtimer_set_msg being set.
- Done doxygen adaption for corresponding vtimer functions.
@kushalsingh007 kushalsingh007 force-pushed the refactor_vtimer_set_msg branch from f93af1e to 1dc6c35 Compare March 18, 2015 22:29
@kushalsingh007
Copy link
Member Author

Changed the commit message.

@kushalsingh007
Copy link
Member Author

All is well

@OlegHahm
Copy link
Member

ACK and go!

OlegHahm added a commit that referenced this pull request Mar 19, 2015
@OlegHahm OlegHahm merged commit 52017a9 into RIOT-OS:master Mar 19, 2015
@kushalsingh007 kushalsingh007 deleted the refactor_vtimer_set_msg branch March 25, 2015 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants