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

Cleanup Temp File Via trap #20

Closed
sshaw opened this issue Feb 13, 2018 · 13 comments
Closed

Cleanup Temp File Via trap #20

sshaw opened this issue Feb 13, 2018 · 13 comments

Comments

@sshaw
Copy link

sshaw commented Feb 13, 2018

I had issues RE #18. When investigating I noticed that the cleanup assumes the script will reach the end. Even without #18 this isn't always the case, i.e., signals. You should so this instead:

trap 'cleanup' 'EXIT'

cleanup() {
    # do cleanup stuff
}

# other code here...

Though I'd also stuff that into the temp directory.

Note that I would have sent a PR, but the instructions for building lead me to believe that this simple PR could turn into a time-consuming endeavor. I see #16 😉

@kaushalmodi
Copy link
Owner

I would have sent a PR, but the instructions for building lead me to believe that this simple PR could turn into a time-consuming endeavor.

Please send a PR.. as we are speaking.. I am working on #16 :D

I'll ping here once done.

@kaushalmodi
Copy link
Owner

kaushalmodi commented Feb 13, 2018

btw.. trap is not available on my system now :P

Also what is that? Haven't googled yet.

@sshaw
Copy link
Author

sshaw commented Feb 13, 2018

Signal handling for bash scripts:

~ >help trap
trap: trap [-lp] [arg signal_spec ...]
    The command ARG is to be read and executed when the shell receives
    signal(s) SIGNAL_SPEC.  If ARG is absent (and a single SIGNAL_SPEC
    is supplied) or `-', each specified signal is reset to its original
    value.  If ARG is the null string each SIGNAL_SPEC is ignored by the
    shell and by the commands it invokes.  If a SIGNAL_SPEC is EXIT (0)
    the command ARG is executed on exit from the shell.  If a SIGNAL_SPEC
    is DEBUG, ARG is executed after every simple command.  If the`-p' option
    is supplied then the trap commands associated with each SIGNAL_SPEC are
    displayed.  If no arguments are supplied or if only `-p' is given, trap
    prints the list of commands associated with each signal.  Each SIGNAL_SPEC
    is either a signal name in <signal.h> or a signal number.  Signal names
    are case insensitive and the SIG prefix is optional.  `trap -l' prints
    a list of signal names and their corresponding numbers.  Note that a
    signal can be sent to the shell with "kill -signal $$".

@kaushalmodi
Copy link
Owner

kaushalmodi commented Feb 13, 2018

I'll ping here once done.

Can you pull the latest master and try below?

make all EMACS=Emacs

By default, EMACS is emacs.

For now, you will need emacs 24.4 or newer, until #21 is resolved.

@kaushalmodi
Copy link
Owner

kaushalmodi commented Feb 14, 2018

I might need your help here.. I cannot get the trap to work.. added this at the end of the script:

debug "Eless Command : $cmd"

echo "here0"

# http://redsymbol.net/articles/bash-exit-traps/
function cleanup {
    echo "here1"
    rm -f "${tempfile}"
}
trap cleanup EXIT

eval "$cmd"

echo "here2"
  • "here1", "here2" never get echoed.. it's as if the script just aborts after the eval "$cmd".

Update: OK, it was because I was running emacs using exec. Removing exec makes the trap work.

@kaushalmodi
Copy link
Owner

kaushalmodi commented Feb 14, 2018

Hello @iqbalansari. Sorry to suddenly drag you into this conversation, especially when you need to go down your memory lane back in April 2017..

Removing exec makes trap work (discussion above).. and also eless works fine as before for various use cases.

I see that exec was added in this commit: 943a513 by you.

Do you have comments on why exec should not be removed?

Thanks!

@kaushalmodi
Copy link
Owner

The trap based cleanup is added to the its-a-trap branch.

@sshaw Do you want to try that out?

@iqbalansari If you too don't mind, can you try out this branch and see what could go wrong now that I call emacs directly in that instead of doing exec emacs ..?

@iqbalansari
Copy link
Contributor

Hi @kaushalmodi,

I had to add the exec call because without that it seems that piping to eless was failing, I will try the its-a-trap branch (nice name 😉 ) and report back

Thanks

@sshaw
Copy link
Author

sshaw commented Feb 14, 2018

make all EMACS=Emacs

/tmp/eless >make all EMACS=Emacs
Emacs binary used: Emacs
Loading /tmp/eless/build/setup-eless.el (source)...
Emacs is now refreshing its package database...
Importing package-keyring.gpg...
Importing package-keyring.gpg...done
Contacting host: orgmode.org:443
Failed to download ‘org’ archive.
Failed to download ‘melpa’ archive.
Package refresh done
Installing ‘org-plus-contrib’ ..
Setting ‘package-selected-packages’ temporarily since "emacs -q" would overwrite customizations
Setting ‘package-selected-packages’ temporarily since "emacs -q" would overwrite customizations
Package ‘org-plus-contrib-’ is unavailable
make: *** [vcheck] Error 255

Which is the same error I get when doing make all.

Instead of trying to make it work with Emacs, I would just try to support older emacs versions e.g., #21.

The trap based cleanup is added to the its-a-trap branch.

Getting the sed/grep errors, but the temp file is gone. 👍

@kaushalmodi
Copy link
Owner

Which is the same error I get when doing make all.

Opened #24 for this.

Instead of trying to make it work with Emacs, I would just try to support older emacs versions e.g., #21.

I agree. Let me know if symlinking approach works for you; then #22 can be closed.

Getting the sed/grep errors, but the temp file is gone. 👍

Thanks for confirming! I am now tracking sed with /I issue ( #23 ) and grep -P issue ( #19 ) separately.

@kaushalmodi
Copy link
Owner

So once @iqbalansari confirms that removing exec doesn't break anything on his side, I will merge this branch and close this issue.

@iqbalansari
Copy link
Contributor

I can confirm that this is working me 👍

@kaushalmodi
Copy link
Owner

Thanks @iqbalansari. Latest master is a trap now.

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

3 participants