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

Fixed with new error #40

Closed
amr66 opened this issue Apr 9, 2015 · 10 comments
Closed

Fixed with new error #40

amr66 opened this issue Apr 9, 2015 · 10 comments
Labels

Comments

@amr66
Copy link
Contributor

amr66 commented Apr 9, 2015

My last commit on amr66/apt now allows to download tinyows and dependencies, but it fails with apache. I got a message from shlex called by get_menu_links(). I didn't try much further, but it seems to be a link in apache.bat.done that causes an exception on link = shlex.split(line)[1]. Error is "Value Error: No closing quatation". Line from bat is:
xxmklink "%OSGEO4W_STARTMENU%\Apache\OSGEO4W-Apache-Monitor.lnk" %OSGEO4W_ROOT%\Apache\bin\ApacheMonitor.exe"
Is this a " which is missing in front of %OSGEO4W_ROOT%?
PS:
I just tried to install with setup exe: It works, even the start menu entries are there,
so by the way: why do we do this: parsing a batch file that has already finished it's work? What intention is behind get_menu_entries?

@amr66
Copy link
Contributor Author

amr66 commented Apr 12, 2015

Annotation: apt remove apache doesn't work either. It's because it installs as a service, so you can't delete open files (eg. httpd.exe) and apt will quit with error. Do we follow the right uninstall way? Do we use preremove-batch files? But Apache has not a preremove. Could we use informaton from get_menu_entries()? Should we warn explicitly with the apache-package? How do we know, that a package installs a service?
I tend to say, apache is an exception and we should use the Start Menu Entry to uninstall the windows service before removing files!
How does setup.exe do this?

@jef-n
Copy link

jef-n commented Apr 12, 2015

I'd consider that a package bug - it should have a preremove and take care of what it installed.

@amr66
Copy link
Contributor Author

amr66 commented Apr 12, 2015

I inspected nearly every tarfile on download.osgeo.org, apache has never had a preremove. So my coriusity remained, how setup.exe deals with it. And, here is the result: It doesn't remove apache!
After some tries, i got this:
setup_remove_apache
;-) So we could do the same and catch the error i had when using apt remove apache...
Where to put this as a request/bug for apache package? Having a preremove would be easy, as they already put the command into win start menü:
C:\OSGeo4W\apache\bin\httpd.exe -k uninstall -n "Apache OSGEO4W Web Server"

@jef-n
Copy link

jef-n commented Apr 12, 2015

Still it's a packaging bug - setup will not remove the service, just the files on reboot. OSGeo4W bugs are filed on OSGeo4W's trac.

@amr66
Copy link
Contributor Author

amr66 commented Apr 12, 2015

Thank you, I found it.
Now we have: two errors in apache-package which are not catched very kindly.

  1. On postinstall we check the syntax of the batch file in get_menu_entries() - and I can't see why we do that. Is this information usefull? And also: Setup's works, apt crashes.
  2. On remove, the service isn't closed, and apt crashes. Can we find a better solution, like setup does?

@maphew maphew added the bug label Apr 13, 2015
@maphew
Copy link
Owner

maphew commented Apr 13, 2015

It's for cleaner uninstall. Apt uses get_menu_links() to capture files created during the install (these aren't present in the .bz2 archive). These are then added to /etc/setup/foo.lst.gz, which is what apt uninstall reads to know what to remove.

I consider failing to creating menu links when setup.exe does a bug, even though the package itself has problems.

@maphew
Copy link
Owner

maphew commented Apr 13, 2015

You correctly identified the troublesome line in the postinstall/apache.bat, but didn't completely catch the nature of the mistake. The preceeding lines are quoting the arguments, not \path\to\httpd.exe. Note the space after .exe in first line:

xxmklink "%OSGEO4W_STARTMENU%\Apache\OSGEO4W-Apache-Stop.lnk" %OSGEO4W_ROOT%\Apache\bin\httpd.exe " -k stop -n \"Apache OSGEO4W Web Server\""
xxmklink "%OSGEO4W_STARTMENU%\Apache\OSGEO4W-Apache-Monitor.lnk" %OSGEO4W_ROOT%\Apache\bin\ApacheMonitor.exe"

...though really not quoting the path as well means the commands will fail on a path with spaces (so, two mistakes). Did you create a packaging ticket on Osgeo4w trac?


Hmm, we have an omission in get_menu_links too: nircmd is used by some packages (qgis) instead of xxmklink for creating shortcuts. ...I'm thinking it would be a good idea to audit this whole series of methods and be confident we aren't missing anything else either.

@maphew
Copy link
Owner

maphew commented Apr 13, 2015

Apt does install apache menu links if the command shell is running in elevated mode. Same for uninstall.

There is also something broken in apache-install.bat, or in it's caller. I get a 'net' is not recognized as an internal or external command,operable program or batch file. error. Net.exe lives in System32 and should be always available.

To defend against malformed batch files in postinstall, or anywhere else for that matter, I think apt will need to record the manifest etc. as it goes, and not just all at the end.

@amr66
Copy link
Contributor Author

amr66 commented Apr 16, 2015

I agree with logging, it is possible through subprocess. But I never tried. And also we can look at the return code, if it's not Null, we shouldn't rename to x.done, may be it can be fixed by the user! We also should give a hint to call remove and try again or something like that.
a propos new error: Package 'xerces-c-vc9' is malformed in setup.ini and causes an error in parse_setup_ini(). It throws an error other than KeyError so I want to add one more exception like this:

try:
    # 'install' and 'source keys have compound values, atomize them
    d['zip_path'],d['zip_size'],d['md5'] = d['install'].split()
except KeyError as e:
    d['zip_path'],d['zip_size'],d['md5'] = ('', '', '')
    if debug:
    print "\n*** Warning: '%s' is missing %s entry in setup.ini. This might cause problems.\n" % (p, e)
except Exception as e:
    print "unknown error parsing package", d
    print "*** %s" % e.message
    d['zip_path'],d['zip_size'],d['md5'] = ('', '', '')

@maphew
Copy link
Owner

maphew commented Apr 25, 2015

I'm marking this closed.

The problem which started this ticket has been resolved, in that apt copes the best it can with installing Apache even though the package itself is broken, and exits gracefully. It also now emits an warning when encountering difficulties parsing archive paths, as per the example above.

With the current structure I don't want to just refuse to rename batch files as .done when the scripts themselves have errors: for example they routinely fail now to create menu and desktop links when not run from an elevated shell. They might also exit with a non-zero code for innocuous errors like A subdirectory or file C:\ProgramData\Microsoft\Windows\Start Menu\Programs\OSGeo4W already exists".

It is problematic that the failures and warnings issued by batch files are intermingled with Apt's own messages. A way forward might be to force batch messages to a separate console and/or log file This would be tracked as a feature enhancement.

More gracefully handling not being in an elevated shell is something I'd like to see someday; that would be a new feature too. ;-)

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

No branches or pull requests

3 participants