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.

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

ocamldebug: load_printer raise uncaught exception when passing directory #6935

Closed
vicuna opened this issue Jul 22, 2015 · 9 comments
Closed

Comments

@vicuna
Copy link

vicuna commented Jul 22, 2015

Original bug ID: 6935
Reporter: RyoheiTokuda
Assigned to: @gasche
Status: closed (set by @xavierleroy on 2017-09-24T15:31:45Z)
Resolution: fixed
Priority: normal
Severity: trivial
Version: 4.02.2
Target version: later
Fixed in version: 4.03.0+dev / +beta1
Category: tools (ocaml{lex,yacc,dep,debug,...})
Tags: patch, junior_job

Bug description

When we pass a directory as an argument of load_printer, uncaught exception Sys_error("Is a directory") is always raised.

Steps to reproduce

OCaml Debugger version 4.02.2

(ocd) load_printer /
Uncaught exception: Sys_error("Is a directory")

Additional information

I have attached a patch to fix the problem.

File attachments

@vicuna
Copy link
Author

vicuna commented Jul 22, 2015

Comment author: @gasche

Review: the patch seems obviously correct.

@vicuna
Copy link
Author

vicuna commented Jul 22, 2015

Comment author: @damiendoligez

The patch is correct but incomplete. For example, you can easily trigger Sys_error "permission denied".
Instead of matching on the error message, it's always better to display it to the user.

@vicuna
Copy link
Author

vicuna commented Nov 17, 2015

Comment author: junsli

Hi, I tried to solve this.

Sys_error message is not consistent: Message for permission denied originally contains filename, but "Is a directory" does not.

Sys_error "./a.out: Permission denied"
Sys_error "Is a directory"

Is it possible to bring str's regexp in debugger build? If so, we can print out generic "name: msg" (if msg does not contain name) or just "msg."

@vicuna
Copy link
Author

vicuna commented Nov 17, 2015

Comment author: @gasche

That seems overkill: just print the filename and the message, and that's ok if there is redundancy.

@vicuna
Copy link
Author

vicuna commented Nov 17, 2015

Comment author: junsli

Hi gasche,

Thanks. Agreed.

I am still new here and getting some low hanging fruit to get me started. How did you do code review and code integrations here? (I didn't find any resources about this online.) Thanks.

@vicuna
Copy link
Author

vicuna commented Nov 17, 2015

Comment author: @gasche

Hi junsli,

Sorry for being a bit terse in the two messages above. The reason we use the "junior_job" tag is precisely to motivate people to start contributing in small but useful ways, and your effort is much appreciated.

There are two ways to do code review:

  • through Mantis: just upload a patch as you have done above, and we can comment on it and, when it feels ready for merging, integrate it.
  • through github: while issues (bug reports) are disabled on the github ocaml repository ( https://github.com/ocaml/ocaml/ ), pull requests are accepted and can be reviewed and merged from the usual github interface.

You are free to use whichever of those systems have your personal preference (github used to be a second-class citizen used for an experiment only, but it is now the official development repository).

Since we are now using git as the version control for the official OCaml distribution repository, the easiest way to preserve authorship information for your patch is to send it in format that "git am" understands (that is, the output of "git format-patch"). This is done automatically by github if you use it, but it is of course also possible to upload a git-friendly patch here on Mantis. Otherwise, if you just upload a plain diff, a maintainer may commit it directly, marking you in the commit message as patch author. In this latter case, it is helpful if you can indicate in the comments somewhere the name under which you would like to be credited.

There are more information on how to contribute to OCaml in the CONTRIBUTING.md document:

https://github.com/ocaml/ocaml/blob/trunk/CONTRIBUTING.md

Thanks for your contribution.

@vicuna
Copy link
Author

vicuna commented Nov 18, 2015

Comment author: junsli

Hi gasche,

Thanks for the explanation. It is clear now.

(I had read the CONTRIBUTING.md, but I saw people only provide patches on Mantis, which obscured some instructions in CONTRIBUTING.md, like send PR, merge request, push changes, etc.)

I'll follow the test process to get familiar with established workflow, and send PR on github.

Thanks again. Cheers.

@vicuna
Copy link
Author

vicuna commented Jan 9, 2016

Comment author: junsli

I forgot to mention that the fix has been integrated into the trunk. The "Target Version" seems wrong to me.

@vicuna
Copy link
Author

vicuna commented Jan 9, 2016

Comment author: @gasche

Thanks! I fixed the bug information.

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

No branches or pull requests

2 participants