Dear LaF0rge, Andreas,
I am really disappointed about the used process for this change. As an absolute minimum run "make check" after bigger changes. The run takes about 20 seconds (and that is probably 15s in the timer test), I have to spend way more time fixing the fall-out from that and it creates the impression that you do not value my time!
I think Andreas should have posted this kind of change to the mailing list and ask for review. The way it was done is really unacceptable and the build is still broken. And to make it worse I think the change is wrong in several ways:
1.) The issue of the last '7bit' being 'empty' only applies to USSD/CB:
"If the total number of characters to be sent equals (8n-1) where n=1,2,3 etc. then there are 7 spare bits at the end of the message. To avoid the situation where the receiving entity confuses 7 binary zero pad bits as the @ character, the carriage return or <CR> character (defined in subclause 7.1.1) shall be used for padding in this situation, just as for Cell Broadcast."
In SMS one has both the octet length and the character length inside the messages. In USSD this information is not present.
2.) The semantic of the change is bad.
+ octet_len = response_len*7/8; + if (response_len*7%8 != 0) + octet_len++; + /* Warning, response_len indicates the amount of septets + * (characters), we need amount of octets occupied */
Every caller of gsm_7bit_encode now needs to consider adding this change, it is better to move this into gsm_utils.c. E.g. take a look at my branch called zecke/features/alpha-numeric for a gsm_7bit_encode_oct. Which comes with a testcase... and moves this responsibility into libosmocore and is based on the real octets written (instead of trying to figure it our afterwards).
We all make money by working on Osmocom sub-projects and I really can't stand such amateurish work. Can we force reset master to before the merge and try again?
cheers holger
On Sun, Jul 07, 2013 at 09:08:26AM +0200, Holger Hans Peter Freyther wrote:
"If the total number of characters to be sent equals (8n-1) where n=1,2,3 etc. then there are 7 spare bits at the end of the message. To avoid the situation where the receiving entity confuses 7 binary zero pad bits as the @ character, the carriage return or <CR> character (defined in subclause 7.1.1) shall be used for padding in this situation, just as for Cell Broadcast."
And the change is incomplete as well:
"If <CR> is intended to be the last character and the message (including the wanted <CR>) ends on an octet boundary, then another <CR> must be added together with a padding bit 0. The receiving entity will perform the carriage return function twice, but this will not result in misoperation as the definition of <CR> in subclause 7.1.1 is identical to the definition of <CR><CR>. The receiving entity shall remove the final <CR> character where the message ends on an octet boundary with <CR> as the last character."
holger
On Sun, Jul 07, 2013 at 09:08:26AM +0200, Holger Hans Peter Freyther wrote:
2.) The semantic of the change is bad.
- octet_len = response_len*7/8;
- if (response_len*7%8 != 0)
octet_len++;- /* Warning, response_len indicates the amount of septets
* (characters), we need amount of octets occupied */
I have reverted the two changes and rebased my zecke/features/alpha-numeric branch. Please review the two commits of the branch. In regard to the USSD fixes I would propose the following:
* We introduce a new API function that will handle the specifics of the USSD packing (last 7bit unused, last 7bit <CR>). The signature could be:
int gsm_7bit_encode_ussd(uint8_t *result, const char *data, int *octets_written); int gsm_7bit_decode_ussd(char *decoded, const uint8_t *user_data, uint8_t length);
* Add a testcase for the above (e.g. '1234567\r' should result in a double \r\r). * Switch the USSD code in libosmocore over
I would argue that writing a regression test is industry standard and basic software engineering but running "make check" after doing a change is absolutely basic.
holger
hi holger,
- We introduce a new API function that will handle the specifics of the USSD packing (last 7bit unused, last 7bit <CR>). The signature could be:
int gsm_7bit_encode_ussd(uint8_t *result, const char *data, int *octets_written); int gsm_7bit_decode_ussd(char *decoded, const uint8_t *user_data, uint8_t length);
i just pushed a branch (jolly/7bit_ussd) i was working on the last hours. it is quite exactly what you prosed.
- Add a testcase for the above (e.g. '1234567\r' should result in a double \r\r).
- Switch the USSD code in libosmocore over
also it includes a test case. the result is similar to your proposal.
currently i pushed everything togehter without propper splitting and commit log message. after review, i can do that.
best regards,
andreas
On Sun, Jul 07, 2013 at 02:41:50PM +0200, Andreas Eversberg wrote:
hi holger,
Dear Andreas,
i just pushed a branch (jolly/7bit_ussd) i was working on the last hours. it is quite exactly what you prosed.
I don't think it needs much splitting my comments are:
* If it helps we can export 'shift' from the common function as well, maybe this simplifies your calculation.
* I would write ('\r' << 1), it is more obvious than 0x1A and modern compilers propagate such constants.
* The decoding, it might be easier to verify this condition on the encoded data?
* In the encoding. If you change octets you also want to modify 'y' as the number of septets.
also it includes a test case. the result is similar to your proposal.
What I don't understand is.. why the first broken version if you know it is not correct? It didn't save any of your time and it wasted plenty of mine.
Holger Hans Peter Freyther wrote:
On Sun, Jul 07, 2013 at 02:41:50PM +0200, Andreas Eversberg wrote:
hi holger,
Dear Andreas,
i just pushed a branch (jolly/7bit_ussd) i was working on the last hours. it is quite exactly what you prosed.
I don't think it needs much splitting my comments are:
- If it helps we can export 'shift' from the common function as well, maybe this simplifies your calculation.
maybe, it will, but this would require export over two functions and still requires some calculations. i think it is simpler to understand the code, when using the number of characters for calculation.
I would write ('\r' << 1), it is more obvious than 0x1A and modern compilers propagate such constants.
The decoding, it might be easier to verify this condition on the encoded data?
In the encoding. If you change octets you also want to modify 'y' as the number of septets.
see my branch.
also it includes a test case. the result is similar to your proposal.
What I don't understand is.. why the first broken version if you know it is not correct? It didn't save any of your time and it wasted plenty of mine.
if i would have known that it is not correct, i would have already fixed it. also it was not clear to me that SMS end USSD encoding is a bit different.
On Sun, Jul 07, 2013 at 03:56:48PM +0200, Andreas Eversberg wrote:
if i would have known that it is not correct, i would have already fixed it. also it was not clear to me that SMS end USSD encoding is a bit different.
Well, just talking about the process (they way things were done). For your initial change you didn't even run "make check" and now after having wasted my time you do the fixup and contribute a testcase to the testsuite you didn't run in the first place. I am not happy about my time being wasted like this.
Dear Holger,
I think you have iterated the point about 'wasted time' sufficiently enough. I understand you being upset, and the point has been driven home to everyone very clearly.
I take responsibility for merging the 7bit changes. The 'make check' was unfortunately only discovered immediately after the push, and I sent a notice to Andreas that this needs fixing.
The discussion of 'stability' vs. 'merging new developments' is a difficult one, and each project has to find its way around that. Linux had parallell stable an development trees for a long time, but this requires a lot more additional effort and people who actually work on 'stable' with time and discipline (and a motivation to do so, which often results from commercial use).
I would personally satte:
1) there is no excuse for not running 'make check' to apply existing test cases against changes that are proposed for merging. No exception here.
2) patches requested to be merged should generally be sent through the applicable mailing list before merging them to master. No merge requests via private e-mail, and not just by mentioning a branch but by posting the entire patch set, patch for patch (git-send-email). This gives them much more visibility to more people.
2) for 'master' I would still want to see a more relaxed attitude than only accepting changes for which comprehensive test cases exist. That is too extreme. We need to balance stability vs. innovation here. Master may break things here and there. It's not a release for production use.
3) We should discuss whether we actually would like to start making formall releases (as opposed to asking everyone to use git master), and/or a stable branch which will only cherry-pick changes from master based on certain guidelines.
Regards, Haradl
Hi Holger and Andreas,
On Sun, Jul 07, 2013 at 06:37:57PM +0200, Holger Hans Peter Freyther wrote:
Well, just talking about the process (they way things were done).
We can establish:
* Andreas should have run 'make check' before asking me to merge * I should have run 'make check' ahead of the commit and not immediately after ;) My apologies for that. * I should have done more careful review of the 7bit changes * merge requests should not go in private mail but on the list, giving the entire team the ability to review before it is merged.
So as a result, we introduce a new rule / process: * merge requests will go through the public mailing list, with the complete patch-set posted via git-send-email and an explicit statement that this is a request for review+merge.
Let's try this for some time and see if we can avoid to cause this problem again. And in general, please try to keep a constructive atmosphere on the list, even under conditions of stress. Nobody here _intentionally_ wants to waste time of others. We have to work together on this, the Osmocom community is small enough that we cannot afford to loose any of you due to frustration.
Regards, Harald
Harald Welte wrote:
We can establish:
- Andreas should have run 'make check' before asking me to merge
- I should have run 'make check' ahead of the commit and not immediately after ;) My apologies for that.
..
So as a result, we introduce a new rule / process:
Maybe consider running 'make check' in a pre-receive hook on the server.
I'm a big fan of Gerrit because it allows such verification to run asynchronously after a push. Gerrit sits between the developer and the official git repository. Everybody pushes to Gerrit, not to the actual repo. (Yes, when using a workflow that depends on gitolite or gitosis access control becomes an issue with Gerrit.)
//Peter
On Mon, Jul 8, 2013 at 11:44 PM, Peter Stuge peter@stuge.se wrote:
Harald Welte wrote:
We can establish:
- Andreas should have run 'make check' before asking me to merge
- I should have run 'make check' ahead of the commit and not immediately after ;) My apologies for that.
..
So as a result, we introduce a new rule / process:
Maybe consider running 'make check' in a pre-receive hook on the server.
I'm a big fan of Gerrit because it allows such verification to run asynchronously after a push. Gerrit sits between the developer and the official git repository. Everybody pushes to Gerrit, not to the actual repo. (Yes, when using a workflow that depends on gitolite or gitosis access control becomes an issue with Gerrit.)
+1. It has much nicer interface for review as well.
-- Regards, Alexander Chemeris. CEO, Fairwaves LLC / ООО УмРадио http://fairwaves.ru
Hi,
Maybe consider running 'make check' in a pre-receive hook on the server.
Not so easy ... it's not good practice to just allow anyone with push privilege to execute code on the server like that. So a good system with proper separation etc ... would be non-trivial. Also, this would slow down pushes to a crawl since you'd need to actually build the thing and its dependencies ...
Cheers,
Sylvain
On Wed, Jul 10, 2013 at 03:00:14PM +0200, Sylvain Munaut wrote:
Hi,
Maybe consider running 'make check' in a pre-receive hook on the server.
Not so easy ... it's not good practice to just allow anyone with push privilege to execute code on the server like that. So a good system with proper separation etc ... would be non-trivial. Also, this would slow down pushes to a crawl since you'd need to actually build the thing and its dependencies ...
as an alternative, offer something like check_patch.pl of Linux kernel to the developer, but which not only checks for indentation/coding style but whihc also does the 'make check'. More reasonable and no load/security on the server.
On Wed, Jul 10, 2013 at 05:52:16PM +0200, Harald Welte wrote:
as an alternative, offer something like check_patch.pl of Linux kernel to the developer, but which not only checks for indentation/coding style but whihc also does the 'make check'. More reasonable and no load/security on the server.
I think it is a social problem, so no technological solution exists. I will add redundancy and reply somewhere else with similar content but from my point of view one should do the following on a branch before asking it to be merged:
1.) rebase against origin/master 2.) Do not mix too many concerns, so maybe create different patch series 3.) go through each change for sanity checking, check the commit message (if the problem fixed is properly described and if a drunk guy in at 10:00 am will understand and have enough context what was attempted to be fixed, I learned this with WebKit and re-learned it with the paging changes in OpenBSC.. so through my own failure) 4.) Make sure that each commit passes make and make check (distcheck) 5.) Ask things to be merged.
So we are all humans and we sometimes don't do the above, sometimes we are lucky and it doesn't get noticed. But in case it is not I think the following should be a social norm:
1.) Fix it 2.) Apologize 3.) Promise to be more careful in the future and attempt to really do it
cheers holger
On Wed, Jul 10, 2013 at 05:52:16PM +0200, Harald Welte wrote:
as an alternative, offer something like check_patch.pl of Linux kernel to the developer, but which not only checks for indentation/coding style but whihc also does the 'make check'. More reasonable and no load/security on the server.
In terms of technology. I see the following options:
* Use the above script and push all commits of a branch (we want to test each commit) to Osmocom test repository and have travis-ci (a Ubuntu VM...) build it and we feed some info back.
* We use gerrit for things we want to stage. Gerrit and a Jenkins would cooperate. There is ACL and user management. Everybody could signup with a OpenID... we could setup a VM with snapshot.. or limit the people that may push..
* Create a script to create debian packages and push them to build.opensuse.org and let them build things for us...
cheers holger
10.07.2013 19:07, Holger Hans Peter Freyther пишет:
- Create a script to create debian packages and push them to build.opensuse.org and let them build things for us...
That would be the best IMO - in addition to smoothing out commit process this will make it much easier to install and maintain everything.
On Wed, Jul 10, 2013 at 9:07 PM, Holger Hans Peter Freyther holger@freyther.de wrote:
We use gerrit for things we want to stage. Gerrit and a Jenkins would cooperate. There is ACL and user management. Everybody could signup with a OpenID... we could setup a VM with snapshot.. or limit the people that may push..
Create a script to create debian packages and push them to build.opensuse.org and let them build things for us...
The latter is always a great thing to do (in addition to having an official PPA for Ubuntu). But it's not a substitution for the former.
PS As I already voted - I believe in the Gerrit way of reviewing.
-- Regards, Alexander Chemeris. CEO, Fairwaves LLC / ООО УмРадио http://fairwaves.ru
Hi Holger,
On Wed, Jul 10, 2013 at 07:07:55PM +0200, Holger Hans Peter Freyther wrote:
In terms of technology. I see the following options:
- Use the above script and push all commits of a branch (we want to test each commit) to Osmocom test repository and have travis-ci (a Ubuntu VM...) build it and we feed some info back.
If at all, I'm more in favor of client-side builds. Something as simple as 'take this branch, use the kernel coding style check_patch script, build each and every commit (locally) and return some status, preferrably by simply sending mail to the user himself. Then that process can be run in background without monitoring output on the console.
- We use gerrit for things we want to stage. Gerrit and a Jenkins would cooperate. There is ACL and user management. Everybody could signup with a OpenID... we could setup a VM with snapshot.. or limit the people that may push..
I'm really against such complex technology. That's not the kind of project I want to be involved in. And I hate web interfaces, they are always slow, require on-line connectivity and the use of a mouse. E-mail is so much more convenient.
- Create a script to create debian packages and push them to build.opensuse.org and let them build things for us...
sorry for being the naysayer again, but I reall dislike all this 'cloud' approach. I'd prefer to keep things simple and local, don't add external dependencies, etc.
Regards, Harald
On Wed, Jul 10, 2013 at 10:16:22PM +0200, Harald Welte wrote:
If at all, I'm more in favor of client-side builds. Something as simple as 'take this branch, use the kernel coding style check_patch script, build each and every commit (locally) and return some status, preferrably by simply sending mail to the user himself. Then that process can be run in background without monitoring output on the console.
I classified this as a social problem because if someone is repeatable not willing to run make and make check on the last commit, I doubt that he will run an extra script as part of his development process.
Please be more careful, check your things because you ask/force others to spend time on it and most importantly learn from your mistakes.
Hi Holger,
On Thu, Jul 11, 2013 at 07:52:08AM +0200, Holger Hans Peter Freyther wrote:
I classified this as a social problem because if someone is repeatable not willing to run make and make check on the last commit, I doubt that he will run an extra script as part of his development process.
I would definitely [want] to use such a script.
From a technical point the above makes sense. Where would you keep the script? Libosmocore and install it as part of it? A new repo?
libosmocore seems fine.
On Thu, Jul 11, 2013 at 08:43:55AM +0200, Harald Welte wrote:
Hi Holger,
On Thu, Jul 11, 2013 at 07:52:08AM +0200, Holger Hans Peter Freyther wrote:
I classified this as a social problem because if someone is repeatable not willing to run make and make check on the last commit, I doubt that he will run an extra script as part of his development process.
I would definitely [want] to use such a script.
A proof of concept requiring git 1.8 is like this (and it includes a todo list).
#!/bin/sh
# Say hello echo "Going to rebase your branch against master and testing"
# TODO: # 1.) Check for git 1.8 for the -x option # 2.) Check/Find checkpatch.pl installed on the system or copy it # 3.) Make it work with OpenBSC and make -C openbsc/ as parameter # 4.) Integrate it to a Makefile.am (like make check-for-submit) # 5.) Install a trap handler to issue a git rebase --abort CHECKPATCH="checkpatch.pl - --no-signoff --ignore LONG_LINE" CMD="make check && make distcheck && (git format-patch --stdout HEAD^1..HEAD | $CHECKPATCH)" BRANCH="origin/master"
EDITOR=/bin/true git rebase -i -x "$CMD" $BRANCH #> /dev/null 2>&1 RES=$?
if [ $RES -eq 0 ]; then echo "All looks fine" else # Execute again... eval $CMD echo "Stuff isn't working!!!! Consider using git rebase --abort" exit 1 fi
-
Alexander Chemeris -
Andreas Eversberg -
Harald Welte -
Holger Hans Peter Freyther -
Peter Stuge -
Sylvain Munaut -
☎