Skip to content

/rust-bindgen

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

Issue #1029: Print error messages if header is a folder or unreadable #1092

Merged
merged 1 commit into from Oct 24, 2017

Conversation

Reviewers

@pepyakin pepyakin

@fitzgen fitzgen

Assignees

@fitzgen fitzgen

Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@seemyvest @highfive @emilio @bors-servo @fitzgen @durka @pepyakin
@seemyvest
Copy link
Contributor

seemyvest commented Oct 23, 2017

r? @fitzgen

@highfive highfive assigned fitzgen Oct 23, 2017

@highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

highfive commented Oct 23, 2017

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @fitzgen (or someone else) soon.

@highfive highfive added the S-awaiting-review label Oct 23, 2017

@pepyakin

pepyakin reviewed Oct 23, 2017
View changes

src/lib.rs Outdated
return Err(());
}
if !can_read(&md.permissions()) {
eprintln!("error: insufficiant permissions to read '{}'", h);

This comment has been minimized.

Sign in to view
Copy link
@pepyakin

pepyakin Oct 23, 2017

Contributor

typo in word insufficient.
src/lib.rs Outdated
#[cfg(unix)]
fn can_read(perms: &std::fs::Permissions) -> bool {
use std::os::unix::fs::PermissionsExt;
perms.mode() & 0o400 > 0

This comment has been minimized.

Sign in to view
Copy link
@pepyakin

pepyakin Oct 23, 2017

Contributor

I'm not sure if this check is reliable enough...
The check assumes that bindgen is running under the user who owns this file which may not be the case.

This comment has been minimized.

Sign in to view
Copy link
@fitzgen

fitzgen Oct 23, 2017

Member

I think perms.mode() & 0o444 > 0 (rather than 0o400) is Good Enough(tm)
@fitzgen

fitzgen approved these changes Oct 23, 2017
View changes

Copy link
Member

fitzgen left a comment

LGTM, just need s/0o400/0o444/ -- thanks @seemyvest !
src/lib.rs Outdated
#[cfg(unix)]
fn can_read(perms: &std::fs::Permissions) -> bool {
use std::os::unix::fs::PermissionsExt;
perms.mode() & 0o400 > 0

This comment has been minimized.

Sign in to view
Copy link
@fitzgen

fitzgen Oct 23, 2017

Member

I think perms.mode() & 0o444 > 0 (rather than 0o400) is Good Enough(tm)

@seemyvest seemyvest force-pushed the seemyvest:fix-1029 branch from bc0e042 to 33a0764 Oct 23, 2017

@emilio

This comment has been minimized.

Sign in to view
Copy link
Collaborator

emilio commented Oct 24, 2017

@bors-servo r=fitzgen

Thanks for working on this!
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Oct 24, 2017

📌 Commit 33a0764 has been approved by fitzgen

@highfive highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 24, 2017

@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Oct 24, 2017

⌛️ Testing commit 33a0764 with merge 7fbedf5...

bors-servo added a commit that referenced this pull request Oct 24, 2017

@bors-servo
Auto merge of #1092 - seemyvest:fix-1029, r=fitzgen 
Issue #1029: Print error messages if header is a folder or unreadable

r? @fitzgen
Loading status checks…
7fbedf5
@bors-servo

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors-servo commented Oct 24, 2017

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing 7fbedf5 to master...

@bors-servo bors-servo merged commit 33a0764 into rust-lang:master Oct 24, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@highfive highfive removed the S-awaiting-merge label Oct 24, 2017

@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Oct 24, 2017

Thanks @seemyvest ! Interested in finding another issue to hack on? I can help find something if nothign from the issue tracker jumps out at you :)

@fitzgen fitzgen referenced this pull request Oct 31, 2017

Closed

Should have better error messages when input files aren't files #1029

@durka

This comment has been minimized.

Sign in to view
Copy link

durka commented Nov 7, 2017

Since this PR, bindgen panics with no explanation if the header does not exist. Consider not unwrapping the result of fs::metadata. This just came up on IRC.
@fitzgen

This comment has been minimized.

Sign in to view
Copy link
Member

fitzgen commented Nov 8, 2017

Woops, I should have caught that in review! Are you interested in making a PR that fixes this @durka ? Or maybe @seemyvest ?
@seemyvest

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

seemyvest commented Nov 9, 2017

No problem 👍

@seemyvest seemyvest referenced this pull request Nov 10, 2017

Merged

Don't unwrap header metadata #1146

bors-servo added a commit that referenced this pull request Nov 10, 2017

@bors-servo
Auto merge of #1146 - seemyvest:fix-metadata, r=emilio 
Don't unwrap header metadata

Fix for a comment in #1092.
r? @fitzgen
Loading status checks…
c54e64c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.