Skip to content

Move socket-stats-enable config to socket-stats-enabled #9067

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

Merged
merged 2 commits into from
Mar 1, 2019

Conversation

kegilbert
Copy link
Contributor

@kegilbert kegilbert commented Dec 12, 2018

Description

This conforms with the other stats config option names.

TODO:

  • Update Example
  • Update Docs

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@deepikabhavnani
Copy link

@kegilbert - Looks good to me, but I have a query: Documents get updated for patch version changes or minor as well?

If we leave it as _enable in document and merge this PR in master it will not match. I would not be much concerned about old feature as most of the times user get aware of the code and check it directly in master, but for new feature should we hold this consistency change for next minor?

@cmonr
Copy link
Contributor

cmonr commented Dec 12, 2018

@ARMmbed/mbed-docs Question about when updates happen

@iriark01
Copy link
Contributor

We don't build master publicly; the public doxygen is always from an official release branch.
We do build master for the test documentation, and that will be updated almost as soon as you merge.

@cmonr
Copy link
Contributor

cmonr commented Dec 14, 2018

I'm fine with this, but from what I can tell, this would need to be pushed out to 5.12. Please correct me if this is wrong.

@deepikabhavnani
Copy link

Yes 5.12

@cmonr
Copy link
Contributor

cmonr commented Dec 21, 2018

@kegilbert I suspect that the example(s) would need to be updated before this could be tested?

@kegilbert
Copy link
Contributor Author

Correct @cmonr

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 16, 2019

@kegilbert What is the status here? Any upcoming updates here as this PR is still open

@kegilbert
Copy link
Contributor Author

Apologies for the delay, I'll update the docs and examples for this.

@kegilbert
Copy link
Contributor Author

Docs: ARMmbed/mbed-os-5-docs#923
Example: ARMmbed/mbed-os-example-socket-stats#8

@cmonr cmonr removed the parked label Jan 17, 2019
Copy link

@deepikabhavnani deepikabhavnani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go 👍

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 18, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 18, 2019

Test run: FAILED

Summary: 1 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM

@alekla01
Copy link
Contributor

restarted ci

@mbed-ci
Copy link

mbed-ci commented Jan 19, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@bulislaw
Copy link
Member

bulislaw commented Feb 26, 2019

This PR is at risk of missing 5.12 release as it's marked as "needs: work". Code freeze is coming! On Friday 1st. Please make necessary updates ASAP and make sure the reviewers are aligned for prompt code inspection.

@cmonr
Copy link
Contributor

cmonr commented Feb 27, 2019

@kegilbert ?

@cmonr
Copy link
Contributor

cmonr commented Feb 27, 2019

#9067 (comment)

#9067 (comment)

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 1, 2019

What's the status here? Two PRs are still opened.

@deepikabhavnani
Copy link

deepikabhavnani commented Mar 1, 2019

What's the status here? Two PRs are still opened

Other PR's can be merged after this is in. This is good to go to CI.

Query - Example PR is up - not in merged state. Should that be merged first for CI to start in this PR?
If I update the example first, other PR's in CI will fail for example.

@cmonr
Copy link
Contributor

cmonr commented Mar 1, 2019

#9067 (comment)

The example needs to be updated first.

If this runs with the current example, the example build will fail because the config parameter changed.

@deepikabhavnani
Copy link

The example needs to be updated first

Example merged

@cmonr
Copy link
Contributor

cmonr commented Mar 1, 2019

CI started

@cmonr
Copy link
Contributor

cmonr commented Mar 1, 2019

#9906 (comment)

Now that the example has been updated, build will start to fail until this is merged into master.

CC @ARMmbed/mbed-os-maintainers

@mbed-ci
Copy link

mbed-ci commented Mar 1, 2019

Test run: FAILED

Summary: 3 of 9 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR8
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARMC5

@deepikabhavnani
Copy link

deepikabhavnani commented Mar 1, 2019

@kegilbert -

"./BUILD/K66F/IAR/.includes_980defce55bb8bfa1a450986b3159977.txt --preinclude=./BUILD/K66F/IAR/mbed_config.h --dependencies BUILD/K66F/IAR/mbed-os/features/netsocket/SocketStats.d -l BUILD/K66F/IAR/mbed-os/features/netsocket/SocketStats.s.txt -o BUILD/K66F/IAR/mbed-os/features/netsocket/SocketStats.o ./mbed-os/features/netsocket/SocketStats.cpp
[Error] SocketStats.cpp@28,35: [Pe135]: class "SocketStats" (declared at line 59 of "/builds/ws/mbed-os-ci_build-IAR8@10/mbed-os/features/netsocket/SocketStats.h") has no member "_mutex"
[Error] SocketStats.cpp@29,24: [Pe135]: class "SocketStats" (declared at line 59 of "/builds/ws/mbed-os-ci_build-IAR8@10/mbed-os/features/netsocket/SocketStats.h") has no member "_stats"
[Error] SocketStats.cpp@30,19: [Pe135]: class "SocketStats" (declared at line 59 of "

@deepikabhavnani
Copy link

Missed this one
https://github.com/ARMmbed/mbed-os/blob/c13052bd5d89936c8b7b7785f1a98e4b66e584f2/features/netsocket/SocketStats.h#L146

This conforms with the other stats config option names
@kegilbert kegilbert force-pushed the socket-stats-name-conform branch 2 times, most recently from 2d3fbcd to 16e0ca9 Compare March 1, 2019 19:48
@cmonr
Copy link
Contributor

cmonr commented Mar 1, 2019

CI started

@kegilbert kegilbert force-pushed the socket-stats-name-conform branch from 16e0ca9 to 194fa12 Compare March 1, 2019 19:54
@cmonr
Copy link
Contributor

cmonr commented Mar 1, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Mar 1, 2019

Test run: FAILED

Summary: 7 of 9 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARMC6
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARMC5
  • jenkins-ci/mbed-os-ci_mbed2-build-ARMC5
  • jenkins-ci/mbed-os-ci_build-IAR8
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR8

@cmonr
Copy link
Contributor

cmonr commented Mar 1, 2019

Ignore #9067 (comment). This is from the last stopped job.

@mbed-ci
Copy link

mbed-ci commented Mar 1, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 5
Build artifacts

@cmonr cmonr merged commit c9192b9 into ARMmbed:master Mar 1, 2019
@cmonr cmonr removed the needs: CI label Mar 1, 2019
@cmonr cmonr mentioned this pull request Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants