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.

Sign up for GitHub

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

Jump to bottom

android: add SUPPORTS_PTHREAD_GETNAME_NP definition #12011

Merged
merged 12 commits into from
Jul 14, 2020
Merged

android: add SUPPORTS_PTHREAD_GETNAME_NP definition #12011

merged 12 commits into from
Jul 14, 2020

Conversation

buildbreaker
Copy link

@buildbreaker buildbreaker commented Jul 9, 2020 •
edited
Loading

Mirroring the PR: #11863

pthread_getname_np is introduced in Android API 26

Introduced here: #11440

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: android: add SUPPORTS_PTHREAD_GETNAME_NP definition
Additional Description:
Risk Level: low
Testing: ci
Docs Changes: n/a
Release Notes: n/a
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

add SUPPORTS_PTHREAD_GETNAME_NP definition
a6998cd 
Signed-off-by: Alan Chiu <achiu@lyft.com>
another if-def
bb595e2 
Signed-off-by: Alan Chiu <achiu@lyft.com>
Alan Chiu added 2 commits July 9, 2020 14:25
opps didnt save
5b6fdc6 
Signed-off-by: Alan Chiu <achiu@lyft.com>
newline
5846c22 
Signed-off-by: Alan Chiu <achiu@lyft.com>
goaway
goaway previously approved these changes Jul 9, 2020
View reviewed changes
@goaway
Copy link
Contributor

goaway commented Jul 9, 2020

@jmarantz think you made the original change, if you don't mind taking a look.

add spelling
e1881fb 
Signed-off-by: Alan Chiu <achiu@lyft.com>
jmarantz
jmarantz reviewed Jul 10, 2020
View reviewed changes
include/envoy/common/platform.h Outdated
@@ -235,3 +235,15 @@ struct mmsghdr {
unsigned int msg_len;
};
#endif

#ifdef __linux__
#define SUPPORTS_PTHREAD_GETNAME_NP
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest defining it to 1 or 0, rather than defining/not-defining it. That would make it less cumbersome to use in logical expressions at call-sites.

I think that's more consistent with other similar definitions in this file.

jmarantz
jmarantz reviewed Jul 10, 2020
View reviewed changes
tools/spelling/spelling_dictionary.txt Outdated
@@ -617,6 +617,7 @@ getaddrinfo
getaffinity
gethostname
getifaddrs
getname
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this is not needed; maybe it was in an intermediate state of this PR?

If I'm wrong go ahead an put it back.

Copy link
Author

Choose a reason for hiding this comment

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

It's complaining for the comment I wrote which uses pthread_getname_np

Copy link
Contributor

Choose a reason for hiding this comment

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

pretty sure you can wrap the word in back ticks (maybe double back ticks?) to avoid the spell checker, i think thats the preferred way to handle these special words

jmarantz
jmarantz previously approved these changes Jul 10, 2020
View reviewed changes
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

Looks fine generally, except for nits.
@envoyproxy/senior-maintainers

pr feedback
6af202b 
Signed-off-by: Alan Chiu <achiu@lyft.com>
better comment
90082ee 
Signed-off-by: Alan Chiu <achiu@lyft.com>
@buildbreaker
Copy link
Author

@jmarantz Thanks for reviewing! I updated the PR with your feedback

@goaway goaway mentioned this pull request Jul 10, 2020
Merged
add np
af10833 
Signed-off-by: Alan Chiu <achiu@lyft.com>
jmarantz
jmarantz previously approved these changes Jul 10, 2020
View reviewed changes
Copy link
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

@envoyproxy/senior-maintainers

snowp
snowp previously requested changes Jul 10, 2020
View reviewed changes
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good just one comment on the spelling stuff

tools/spelling/spelling_dictionary.txt Outdated
@@ -617,6 +617,7 @@ getaddrinfo
getaffinity
gethostname
getifaddrs
getname
Copy link
Contributor

Choose a reason for hiding this comment

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

pretty sure you can wrap the word in back ticks (maybe double back ticks?) to avoid the spell checker, i think thats the preferred way to handle these special words

single back ticks
fe5cf72 
Signed-off-by: Alan Chiu <achiu@lyft.com>
more backticks
5ac5ef9 
Signed-off-by: Alan Chiu <achiu@lyft.com>
jmarantz
jmarantz previously approved these changes Jul 10, 2020
View reviewed changes
@jmarantz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@buildbreaker
Copy link
Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 12011 in repo envoyproxy/envoy

@jmarantz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Merge branch 'master' of https://github.com/envoyproxy/envoy into add…
bf90d74 
…-supports_pthread_getname_np-definition

Signed-off-by: Alan Chiu <achiu@lyft.com>
@junr03
Copy link
Member

junr03 commented Jul 14, 2020

/azp run

@azure-pipelines
Copy link

Pull request contains merge conflicts.

Merge branch 'master' of https://github.com/envoyproxy/envoy into add…
354fa04 
…-supports_pthread_getname_np-definition

Signed-off-by: Alan Chiu <achiu@lyft.com>
@buildbreaker
Copy link
Author

@jmarantz Thank you for the approval again!

@junr03
Copy link
Member

junr03 commented Jul 14, 2020

The build is failing for known issues outside of this PR: #12067 and #12071. I'll merge this

@mattklein123 mattklein123 merged commit d27fd11 into envoyproxy:master Jul 14, 2020
@buildbreaker
Copy link
Author

thank you @junr03 @mattklein123 !

@buildbreaker buildbreaker deleted the add-supports_pthread_getname_np-definition branch July 14, 2020 17:00
This was referenced Jul 14, 2020
Closed
Closed
Merged
scheler pushed a commit to scheler/envoy that referenced this pull request Aug 4, 2020
android: add SUPPORTS_PTHREAD_GETNAME_NP definition (envoyproxy#12011)
9618c37 
Signed-off-by: Alan Chiu <achiu@lyft.com>
Signed-off-by: scheler <santosh.cheler@appdynamics.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keith keith keith left review comments

@jmarantz jmarantz jmarantz approved these changes

@goaway goaway goaway left review comments

@snowp snowp snowp left review comments

Assignees

@jmarantz jmarantz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@buildbreaker @goaway @jmarantz @junr03 @keith @snowp @mattklein123