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

When on the CLI, catch ctrl-c early in execution and exit cleanly #234

Merged
merged 10 commits into from Jan 6, 2018
Merged

When on the CLI, catch ctrl-c early in execution and exit cleanly #234

merged 10 commits into from Jan 6, 2018

Conversation

eode
Copy link
Contributor

@eode eode commented Dec 10, 2017

Normally I'd just do a try: except: block on or in main(), but at load-time, we load a bunch of external modules that take a lot of time, during which ctrl-c will cause an exception that misses that block. ..so, this was added to quilt/__init__.py.

Also, sometimes during development we want to be able to see the traceback -- for example, if we're wondering what's taking so long, or what function is causing network activity. Toward that end, there's now a variable quilt._DEV_MODE which enables/disables traceback.

If your first argument is --dev, or if the environment has QUILT_DEV_MODE=True, tracebacks will still be shown. Help for --dev is suppressed and doesn't show up in quilt help, quilt --help, etc.

@eode
Copy link
Contributor Author

eode commented Dec 10, 2017

Ha! Caught by my own test.

@asah
Copy link
Contributor

asah commented Dec 10, 2017 •
edited

pls move the comment about try/except vs init into the code, as a comment. This helps future maintainers.

otherwise, looks good to me (lgtm) -- pls check with @dimaryaz as well.

@asah asah requested a review from dimaryaz December 10, 2017 22:09
@eode
full testing for kb interrupt, misc fixes, exit code
4e44484 
* Removed unecessary import in main.py
* Exiting with ctrl-c produces a unique exit code (stored in const)
* Noticed a bug in MockObject where exceptions were substituted, but
shouldn't be.  Fixed.
* Testing ensures '--dev' is accepted
* Testing ensures normal-mode traceback suppression
* Testing ensures dev-mode traceback display
@eode
Expand comment re: using signal vs try/except
0cfdb98
@eode
Copy link
Contributor Author

eode commented Dec 11, 2017

I'm not sure if it's ok for me to touch const.py, let me know if this isn't the best place for exit codes.

I've considered that an 'errors.py' might be useful, which could include (all) exceptions and error codes.

@quiltdata
Copy link
Collaborator

quiltdata commented Dec 11, 2017 via email

asah
asah reviewed Dec 11, 2017
View reviewed changes
compiler/quilt/test/test_cli.py
return result
return self.execute_fast(cli_args)

def execute_cli(self, cli_args):
Copy link
Contributor

Choose a reason for hiding this comment

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

dumb q: I thought [something like this] was already committed as part of the CLI testing? sorry if dumb q...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a dumb question -- that's just the original function, split into two functions, so that I could call the execute_cli() one separately. I ended up needing to use Popen, anyways, so it was moot. ..but I left the change in in case some future test has reason to prefer either execute_cli or execute_fast over using execute, which auto-selects cli or fast mode based on environment.

@eode
Copy link
Contributor Author

eode commented Dec 11, 2017

One sec, I have a between-python-versions (or similar) portability issue that's not passing appveyor. I'll push a fix momentarily.

@eode
Copy link
Contributor Author

eode commented Dec 11, 2017

I've pushed that change, but appveyor looks stuck (on a different test).

@eode
Copy link
Contributor Author

eode commented Dec 11, 2017

I've removed some code to reset appveyor, but it looks like they're backlogged or something similar. In the mean time, don't merge this branch until I put the code back in and re-test that, even if appveyor passes in the mean time.

@eode
Copy link
Contributor Author

eode commented Dec 11, 2017

appveyor has made a new test, but at this point at least one test is still hung (https://ci.appveyor.com/project/quiltdata/quilt-compiler/build/1.0.102, second test, at 52 minutes currently).

@eode eode mentioned this pull request Dec 11, 2017
Merged
@eode
Copy link
Contributor Author

eode commented Dec 11, 2017

Bad news: Appveyor is hung again. Looks like it can't handle ctrl-c events in testing.
Good news: That means we know why, and can prevent it from hanging by just xfailing the test on windows.

Amusingly, appveyor found a bug, and the fix led to the appveyor hang.

In any case, the latest commit, 78cfca4, should still be executed on appveyor, but any running tests on this branch before that should be cancelled, if possible -- or, they'll drop off on their own eventually.

The current fix xfails a suspect portion of code. If that doesn't work, I'll xfail the whole test in another commit.

@quiltdata
Copy link
Collaborator

quiltdata commented Dec 11, 2017 via email

@eode
Copy link
Contributor Author

eode commented Dec 11, 2017

ready to roll.

dimaryaz
dimaryaz suggested changes Dec 11, 2017
View reviewed changes
Copy link
Contributor

@dimaryaz dimaryaz left a comment

Choose a reason for hiding this comment

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

__init__.py looks like the wrong place to do it. Figuring out the executable name is hacky, and it's risky if we get it wrong and set the interrupt handler when we shouldn't.

I think our own entry point is the place to do it - i.e., the quilt script that does if __name__ == '__main__'. It's as early as it gets, and it only runs when quilt is run from the CLI.

Not sure if setup.py has a way for us to customize the script... But even if it doesn't, it should be possible to wrap main, or maybe write our own entry point script.

@eode
Copy link
Contributor Author

eode commented Dec 12, 2017 •
edited

  • Using the name we're called as to implement traceback suppression is about as hacky as using args. In fact, a lot of tools do just this in order to "pretend" to be a different program or act differently when called by a different name. Swiss-army-knife shell tools, for example, or quite a few MTA's out there that act as sendmail when called as sendmail.
  • We've previously decided that quilt must be installed when executed, so figuring out valid executable name by entry points is not only not hacky, it's fully supported, and usages of pkg_resources like this are common, both for a program referencing itself, and to add plugins to a program.
  • The only cases I can think of where there's a conflict are:
    • Quilt is executed via a different name, by being linked to, in which case tracebacks would show if ctrl-c is used. Not really a problem.
    • Quilt is imported by another python program named quilt. At which point, we would suppress tracebacks for both us and them. Serves them right. ..and we're already quilt on PYPI, which is about as officially owning-the-name as it gets.
    • Quilt is imported by another program that has mangled sys.argv so that 'quilt' is the first argument, at which point we would suppress tracebacks for both us and them. Serves them right -- if they're changing sys.argv to make it look like they're running as a different program (which they're importing) then they'd better know what they're doing. ..and, this is pretty unlikely.

The pure and simple of it is, not only is it reasonable and supported, the consequences are minimal if we're incorrect. On top of that, the most inconvenient case is also the most improbable -- and we can solve it by a line of documentation somewhere about external usage:

"Quilt does not support being imported by a program that spoofs sys.argv[0]. If you do so, it may suppress your tracebacks by replacing the interrupt handler for SIGINT."

..but really, we probably don't even need that, as it's just going to be noise to most people.

In any case, I know of no mechanism for altering the actual entry point script, which is auto-generated by setuptools and/or pip during install, depending on the specifics of the system. If you know one, please let me know, because I'd like to know, it would be a cool tidbit to be aware of.

Lastly, if we forego the entry-points system altogether (not recommended for more than one reason I'll not go into here), and use our own script, then we also must detect all of the interoperability differences between systems that many talented people in the pip and setuptools contributor pool have already found and fixed.

..and __init__.py is the first thing read after the entry point script. EDIT: To be clear, I mean the entry point script executed by the user, created by setuptools, which imports quilt and then runs the entry point function or module we specify. So the load order is: <setuptools entry point script>, quilt/__init__.py, quilt/tools/__init__.py, quilt/tools/main.py, quilt.tools.main(). quilt.tools.main() is the entry point, and quilt is what I mean by the entry point script. The large modules are loaded in quilt/__init__.py currently.

We could, however, implement lazy loading of modules, and move exception handling into a try: except: block in a new __main__.py file or some such.

@eode
Copy link
Contributor Author

eode commented Dec 12, 2017

This still needs to be tested on Windows. I'll try to check into that when there's less that's pressing.

@asah
Copy link
Contributor

asah commented Dec 16, 2017 •
edited

ping re windows (if hard, then OK to defer windows support - add a comment)

@eode
Copy link
Contributor Author

eode commented Dec 18, 2017

I'll look into it once I've got something useful on quilt export, so maybe the 20th, or I'll start on it on the 23rd.

@asah
Copy link
Contributor

asah commented Dec 19, 2017

@dimaryaz @eode what's the status of this PR?

@eode
Copy link
Contributor Author

eode commented Dec 19, 2017 •
edited

As far as I'm concerned, it's ready to go except that it hasn't been tested on Windows, and the specific test has been skipped because it crashes the Appveyor test. ..although, ironically Appveyor caught a related bug, and the fix led to the Appveyor test skip.

Aside from that, I consider it done, but I can change it if @dimaryaz still wants a change.

If it's here when my current TODO list is done, or if it's bumped up in priority by you @asah , I'll test it on Windows and check that off.

@eode
Copy link
Contributor Author

eode commented Jan 5, 2018

Well, it'd be a pain to actually get the test working on Windows, and it's probably not worth the effort. However, I've manually verified that this works correctly. Unless we want to put additional time into the test re: windows, I'm good with this. @dimaryaz ?

@quiltdata
Copy link
Collaborator

quiltdata commented Jan 6, 2018 via email

@asah asah merged commit 477137d into quiltdata:master Jan 6, 2018
@eode eode deleted the eode-fix-ctrl-c-traceback branch January 6, 2018 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asah asah asah left review comments

@dimaryaz dimaryaz dimaryaz requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@eode @asah @quiltdata @dimaryaz