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

Realtek: serial - line ending fix #6450

Merged
merged 1 commit into from Mar 26, 2018

Conversation

Projects
None yet
8 participants
@0xc0170
Member

0xc0170 commented Mar 26, 2018

Description

One line had a window line ending in Realtek HAL Serial implementation.

To reproduce, do a change in a file, and this line gets shown (line change to unix line endings):

 git diff
warning: CRLF will be replaced by LF in targets/TARGET_Realtek/TARGET_AMEBA/TARGET_RTL8195A/serial_api.c.
The file will have its original line endings in your working directory.
diff --git a/targets/TARGET_Realtek/TARGET_AMEBA/TARGET_RTL8195A/serial_api.c b/targets/TARGET_Realtek/TARGET_AMEBA/TARGET_RTL8195A/serial_api.c
index 5f1cccb6f..46f2dde5f 100644
--- a/targets/TARGET_Realtek/TARGET_AMEBA/TARGET_RTL8195A/serial_api.c
+++ b/targets/TARGET_Realtek/TARGET_AMEBA/TARGET_RTL8195A/serial_api.c
@@ -327,7 +327,7 @@ void serial_irq_set(serial_t *obj, SerialIrq irq, uint32_t enable)
         if(enable) {
             if (irq == RxIrq) {
                 log_uart_irq_set(&stdio_uart_log, IIR_RX_RDY, enable);
-                serial_log_irq_en |= SERIAL_RX_IRQ_EN;
+                serial_log_irq_en |= SERIAL_RX_IRQ_EN;
             } else {
                 log_uart_irq_set(&stdio_uart_log, IIR_THR_EMPTY, enable);
                 serial_log_irq_en |= SERIAL_TX_IRQ_EN;

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

Realtek: serial - line ending fix
One line had a window line ending
@geky

geky approved these changes Mar 26, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 26, 2018

To confirm, here is a view at line endings on current master

            if (irq == RxIrq) {¬
                log_uart_irq_set(&stdio_uart_log, IIR_RX_RDY, enable);¬
                serial_log_irq_en |= SERIAL_RX_IRQ_EN;CR
¬
            } else {¬
                log_uart_irq_set(&stdio_uart_log, IIR_THR_EMPTY, enable);¬
                serial_log_irq_en |= SERIAL_TX_IRQ_EN;¬
            }¬

The line has CR.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 26, 2018

/morph build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 26, 2018

Fixes #6449

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 26, 2018

Build : SUCCESS

Build number : 1567
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6450/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 26, 2018

@OPpuolitaival Please review pr-head

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Mar 26, 2018

As told earlier.. the whitespace change broke the CI, and therefore CI cannot build even this one.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Mar 26, 2018

Sounds like the CI is too easily broken in that case :(

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Mar 26, 2018

What?

This is now broken on all our workspaces.. not just CI.

If you have cloned mbed-os earlier and checkout master branch and then try to check out some other branch, you will see the same issue.

Problem is that if you Git is (properly) configured to convert line endings, then you see this issue.

If your Git is NOT configured, then all our files are randomly either Windows or Unix line endings. Depending on who touched that file previously.

@SeppoTakalo

This comment has been minimized.

Contributor

SeppoTakalo commented Mar 26, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Mar 26, 2018

As told earlier.. the whitespace change broke the CI, and therefore CI cannot build even this one.

Shouldn't we just get this merged then?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 26, 2018

Based on the impact, I am happy to get this in (one line change, no code change, only line ending). the flow in our CI is correct (git checkout branches, I got now the same problem locally as was switching between branches ), we will need to review gitattributes or other options to stay away from this line endings breakages.

@adbridge adbridge merged commit d97d55a into ARMmbed:master Mar 26, 2018

11 of 12 checks passed

continuous-integration/jenkins/pr-head This commit cannot be built
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/ble-host-tests Local ble-host-tests testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9055 cycles (-532 cycles)
Details
travis-ci/littlefs Passed, code size is 10092B
Details
travis-ci/tools Local tools testing has passed
Details

@adbridge adbridge removed the needs: CI label Mar 26, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 3, 2018

If someone does manage to bypass/ignore .gitattributes and check in an unnormalised file, Git clients that do obey it get confused when they see the unnormalised file in the repo.

Basically they immediately try to normalise it in the work area, which marks the file as locally changed, which blocks some commands.

It's unfortunate that the normal CI sequence for the PR that breaks it doesn't see the problem, while the CI for subsequent PRs does.

You could use astyle to do a line-ending check, but I think the CI should (also?) programmatically use Git to check for this specific normalisation problem.

The return code of git diff-files --quiet immediately after a fresh check-out would indicate the problem - it will return non-zero indicating you have a local change (due to .gitattributes normalisation), which you wouldn't have if the checked-in files were normalised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment