check conditions for .signac_shell_history permission. #279

ac-optimus · 2020-01-28T22:05:57Z

Description

Motivation and Context

This will fix - #262

Types of Changes

Documentation update
Bug fix
New feature
Breaking change¹

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

I am familiar with the Contributing Guidelines.
I agree with the terms of the Contributor Agreement.
My name is on the list of contributors.
My code follows the code style guideline of this project.
The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:

I have updated the API documentation as part of the package doc-strings.
I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

Example for a changelog entry: Fix issue with launching rockets to the moon (#101, #212).

csadorf

@ac-optimus Thank you for the contribution!

In addition to my code comments, could you make sure that your code follows our code style guidelines? The easiest way to ensure that is by using flake8, ideally in combination with a git hook, see here for instructions on how to set that up.

signac/__main__.py

bdice · 2020-01-29T20:30:00Z

signac/__main__.py

                        atexit.register(readline.write_history_file, fn_hist)
+                    except PermissionError:
+                        print("Warning: .signac_shell_history does not have read/write permission.\


Try breaking the line like this. Python will combine the two strings into one message. Notice there is a space at the end of the first line.

print("Warning: .signac_shell_history does not have read/write permission. " "The history of this shell will not be saved.")

@bdice done!

csadorf

Thank you for making that change! I have a few more questions.

csadorf · 2020-01-30T08:55:22Z

signac/__main__.py

-                            raise
-                    atexit.register(readline.write_history_file, fn_hist)
+                        with open(fn_hist, 'w') as f: pass
+                        atexit.register(readline.write_history_file, fn_hist)


Should we maybe just open it in append (a) mode so that in case that we fail to write the history from memory at program exit it is at least not lost?

@csadorf yes seems like a good idea. done that!

@csadorf I'm not sure if I understand. How does using a fix the problem if we don't have write access? I thought the primary difference between w and a was whether it overwrote the existing file or appended to the end.

@bdice actually the main reason for line-1048 is to check weather the file has a write permission. If it doesn't then we get an exception that is used to throw a warning.

@ac-optimus @csadorf I would prefer to use the approach of permissions-checking for this specific instance, instead of attempting to write to the file and catching an exception. I believe that opening the file (using with open(...): pass) requires more system calls and may have unintended side-effects such as memory allocation (for the file buffer) and updating the "modified" timestamp of the file with no actual modifications. My preferred solution would be to use os.access as before.

or we can throw common warning for both read and write permission error like this-

try: readline.read_history_file(fn_hist) readline.set_history_length(1000) with open(fn_hist, "a"): pass except FileNotFoundError: pass except PermissionError: print("Warning: Shell history from " "{}".format(os.path.relpath(fn_hist)), "would be lost. Permission error.") atexit.register(readline.write_history_file, fn_hist)

I'd just go with the last alternative, since I believe that it solves the original issue. @bdice Do you agree?

I recommend that we adopt the snippet I proposed above: #279 (comment)
@ac-optimus, can you apply that snippet and test it out manually? (no need to write a unit test)

@bdice Sounds good to me!

@bdice I already tried it manually and it was the reason why I proposed slight modification (1st code spinet above). readline.write_history_file(fn_hist) does not throw error when we do not have write permission and hence the exception is never called. And the 3rd one is more general. Please have a look and correct me if I am wrong.

signac/__main__.py

This reverts commit a308fd5.

signac/__main__.py

bdice · 2020-01-30T19:26:11Z

signac/__main__.py

-                            raise
-                    atexit.register(readline.write_history_file, fn_hist)
+                        with open(fn_hist, 'w') as f: pass
+                        atexit.register(readline.write_history_file, fn_hist)


@csadorf I'm not sure if I understand. How does using a fix the problem if we don't have write access? I thought the primary difference between w and a was whether it overwrote the existing file or appended to the end.

Co-Authored-By: Bradley Dice <[email protected]>

signac/__main__.py

csadorf · 2020-01-31T08:28:51Z

@ac-optimus I recommend that you execute the linter locally so that your code follows the code style guidelines.

ac-optimus · 2020-01-31T17:21:16Z

@ac-optimus I recommend that you execute the linter locally so that your code follows the code style guidelines.

okay sure.

bdice · 2020-01-31T17:38:02Z

Edit: This was in response to "what to do now?" -- sorry, I thought that was a general question. I will reply to the comment where you specifically wrote that question.

@ac-optimus Your code still isn't passing the linter (flake8). Try installing the flake8 hook as described here, which prevents you from committing code that fails the linter. You can see the errors in the CI output.

signac/__main__.py:1048:52: F841 local variable 'f' is assigned to but never used
                        with open(fn_hist, 'w') as f: pass
                                                   ^
signac/__main__.py:1048:53: E701 multiple statements on one line (colon)
                        with open(fn_hist, 'w') as f: pass
                                                    ^
signac/__main__.py:1054:101: E501 line too long (112 > 100 characters)
                               "The history of this shell will not be saved.").format(os.path.relpath(fn_hist)))

ac-optimus · 2020-01-31T18:10:06Z

Edit: This was in response to "what to do now?" -- sorry, I thought that was a general question. I will reply to the comment where you specifically wrote that question.

@ac-optimus Your code still isn't passing the linter (flake8). Try installing the flake8 hook as described here, which prevents you from committing code that fails the linter. You can see the errors in the CI output.

signac/__main__.py:1048:52: F841 local variable 'f' is assigned to but never used
                        with open(fn_hist, 'w') as f: pass
                                                   ^
signac/__main__.py:1048:53: E701 multiple statements on one line (colon)
                        with open(fn_hist, 'w') as f: pass
                                                    ^
signac/__main__.py:1054:101: E501 line too long (112 > 100 characters)
                               "The history of this shell will not be saved.").format(os.path.relpath(fn_hist)))

@bdice @csadorf corrected the code style(using linter). Need to learn a lot. Thanks!
Should I merge all the commits, seems like lot mess happened due to me?

bdice · 2020-01-31T18:14:21Z

@ac-optimus We will squash all the commits on this branch into one when the pull request is merged so don't worry about the history being unclean. We can wait until the end to merge in master. Take a look at the code block I suggested on that comment thread and let me know what you think. I think it resolves the questions that @csadorf and I both had about EAFP.

codecov · 2020-01-31T18:23:47Z

Codecov Report

Merging #279 into master will decrease coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #279      +/-   ##
==========================================
- Coverage   64.88%   64.81%   -0.07%     
==========================================
  Files          40       40              
  Lines        5624     5630       +6     
==========================================
  Hits         3649     3649              
- Misses       1975     1981       +6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e55e1a...ce499d7. Read the comment docs.

csadorf

I did not know that the permissions would be automatically changed/ the file is replaced. Let's focus on the original issue (issue #262), that is that the shell fails to launch when there is a permissions error. Instead a warning should be printed and the shell is opened anyways. Whether we can also save the history is a different problem. I agree with @bdice and suggest we don't preemptively add any checks concerning that.

csadorf · 2020-02-13T12:53:01Z

@bdice It looks like this PR is waiting on your input. @ac-optimus Is that correct?

ac-optimus · 2020-02-13T12:54:57Z

@bdice It looks like this PR is waiting on your input. @ac-optimus Is that correct?

@csadorf yes I guess.

csadorf

Please adopt the code snippet that @bdice proposed. The original issue will be addressed by that. See also my in-code comment.

csadorf

I need to revise my previous review comment, I'm going to push the needed modifications to this PR.

csadorf

After more testing, I found that the write_history_file function will throw if the file can actually not be overridden by the user. In conclusion, @ac-optimus please adopt @bdice 's proposed solution exactly. Thank you.

ac-optimus · 2020-02-17T16:48:00Z

After more testing, I found that the write_history_file function will throw if the file can actually not be overridden by the user. In conclusion, @ac-optimus please adopt @bdice 's proposed solution exactly. Thank you.

okay sure!

csadorf · 2020-02-17T17:11:11Z

@ac-optimus It looks like there is some issue with our CI configuration. This sometimes happens when libraries that we depend on are updated in a non-backwards-compatible way. So please ignore the failing CI for now.

csadorf · 2020-02-17T17:11:46Z

@ac-optimus Please merge master, this might also fix the CI issues.

ac-optimus · 2020-02-17T17:20:59Z

@ac-optimus Please merge master, this might also fix the CI issues.

@csadorf merged it to master of my fork.

csadorf · 2020-02-17T17:25:07Z

@ac-optimus Please merge master, this might also fix the CI issues.

@csadorf merged it to master of my fork.

Looks like the master branch of your fork is out of date.

ac-optimus · 2020-02-17T17:42:29Z

@ac-optimus Please merge master, this might also fix the CI issues.

@csadorf merged it to master of my fork.

Looks like the master branch of your fork is out of date.

corrected that! the master is updated and merged with the permission_issue branch.

csadorf

LGTM! Thx!

csadorf · 2020-02-18T09:34:45Z

@atravitz I fixed the CI issues upstream and merged master here. Please merge this PR at your discretion.

check conditions for .signac_shell_history permission

9b7b488

ac-optimus requested review from a team as code owners January 28, 2020 22:05

ac-optimus requested review from atravitz and removed request for a team January 28, 2020 22:05

csadorf self-requested a review January 29, 2020 08:16

csadorf requested changes Jan 29, 2020

View reviewed changes

signac/__main__.py Show resolved Hide resolved

bdice assigned ac-optimus Jan 29, 2020

changing to try-except stype

26812c3

ac-optimus removed their assignment Jan 29, 2020

ac-optimus requested a review from csadorf January 29, 2020 20:19

bdice reviewed Jan 29, 2020

View reviewed changes

correcting the print statement.

a3b646e

ac-optimus requested a review from bdice January 29, 2020 21:15

csadorf reviewed Jan 30, 2020

View reviewed changes

ac-optimus added 3 commits January 31, 2020 00:13

suggested changes

a308fd5

Revert "suggested changes"

12501f0

This reverts commit a308fd5.

suggested chagnes.

8771400

ac-optimus requested a review from csadorf January 30, 2020 18:59

bdice reviewed Jan 30, 2020

View reviewed changes

suggested changes related to white-space.

a2ff918

Co-Authored-By: Bradley Dice <[email protected]>

ac-optimus requested a review from bdice January 30, 2020 19:44

bdice reviewed Jan 30, 2020

View reviewed changes

signac/__main__.py Outdated Show resolved Hide resolved

removing unwanted line

d1610da

ac-optimus requested a review from bdice January 30, 2020 20:06

correcting the code style

676778f

changing write(w) to append(a)

d29ffc0

csadorf reviewed Feb 3, 2020

View reviewed changes

csadorf requested changes Feb 17, 2020

View reviewed changes

csadorf removed the request for review from bdice February 17, 2020 15:00

csadorf added the bug Something isn't working label Feb 17, 2020

csadorf reviewed Feb 17, 2020

View reviewed changes

csadorf requested changes Feb 17, 2020

View reviewed changes

adding suggested changes

27bac18

ac-optimus requested a review from csadorf February 17, 2020 17:01

Merge branch 'master' into permission_issue

71da724

csadorf approved these changes Feb 18, 2020

View reviewed changes

Merge branch 'master' into permission_issue

40675ad

bdice approved these changes Feb 19, 2020

View reviewed changes

atravitz approved these changes Feb 19, 2020

View reviewed changes

Update changelog.txt

ce499d7

atravitz merged commit 9f212d8 into glotzerlab:master Feb 19, 2020

vyasr mentioned this pull request Feb 26, 2020

Permissions on .signac_shell_history give cryptic error #262

Closed

bdice added this to the v1.4.0 milestone Feb 27, 2020

check conditions for .signac_shell_history permission. #279

check conditions for .signac_shell_history permission. #279

Conversation

ac-optimus commented Jan 28, 2020 • edited by atravitz Loading

Description

Motivation and Context

Types of Changes

Checklist:

csadorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csadorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ac-optimus Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csadorf commented Jan 31, 2020

ac-optimus commented Jan 31, 2020

bdice commented Jan 31, 2020 • edited Loading

ac-optimus commented Jan 31, 2020

bdice commented Jan 31, 2020 • edited Loading

codecov bot commented Jan 31, 2020 • edited Loading

Codecov Report

csadorf left a comment

Choose a reason for hiding this comment

csadorf commented Feb 13, 2020

ac-optimus commented Feb 13, 2020

csadorf left a comment

Choose a reason for hiding this comment

csadorf left a comment

Choose a reason for hiding this comment

csadorf left a comment

Choose a reason for hiding this comment

ac-optimus commented Feb 17, 2020 • edited Loading

csadorf commented Feb 17, 2020

csadorf commented Feb 17, 2020

ac-optimus commented Feb 17, 2020

csadorf commented Feb 17, 2020

ac-optimus commented Feb 17, 2020

csadorf left a comment

Choose a reason for hiding this comment

csadorf commented Feb 18, 2020

ac-optimus commented Jan 28, 2020 •

edited by atravitz

Loading

ac-optimus Jan 31, 2020 •

edited

Loading

bdice commented Jan 31, 2020 •

edited

Loading

bdice commented Jan 31, 2020 •

edited

Loading

codecov bot commented Jan 31, 2020 •

edited

Loading

ac-optimus commented Feb 17, 2020 •

edited

Loading