Skip to content
This repository has been archived by the owner on Oct 26, 2023. It is now read-only.

HTTP Port + working Python3 #147

Closed
wants to merge 8 commits into from
Closed

HTTP Port + working Python3 #147

wants to merge 8 commits into from

Conversation

lathama
Copy link
Contributor

@lathama lathama commented Mar 5, 2017

Rory, I had to add the HTTP Port for testing and got Python3 working. I need some help testing the NBD stuff but am happy to correct/update for that.

Tell me what you want to do here. If you want to just do that HTTP Port that is cool.

example from setuptools install...
Copying PyPXE-1.7.2-py3.4.egg to /usr/local/lib/python3.4/dist-packages
Adding PyPXE 1.7.2 to easy-install.pth file
Installing pypxe script to /usr/local/bin

Installed /usr/local/lib/python3.4/dist-packages/PyPXE-1.7.2-py3.4.egg
Processing dependencies for PyPXE==1.7.2
Finished processing dependencies for PyPXE==1.7.2

@@ -122,7 +122,9 @@ def __init__(self, **server_settings):
import_safe[packed_mac] = imported[lease]
self.leases.update(import_safe)
self.logger.info('Loaded leases from {0}'.format(self.save_leases_file))
except IOError, ValueError:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the rationale for splitting these up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically I should have made those proper functions but this gets by the Python3 test for exception changes.

@psychomario
Copy link
Collaborator

HTTP port addition looks good, I'm surprised it wasn't already there to be honest.

wrt NBD, IIRC I used the Arch wiki to setup a initrd/kernel that would boot from an arch disk image: https://wiki.archlinux.org/index.php/Diskless_system, but I imagine there is documentation for whatever your distro is.

Unfortunately I don't have the time at the moment to test these things to get them merged, so unless @mmattioli is willing and has the time, they might have to wait until June ish when I've graduated, apologies for that. I don't mind if your PRs pile up in the mean time, although I realise that might be a little awkward to manage.

I notice you've imported the io package, what's this for? According to both the python2 and python3 documentation:

The builtin open function is defined in this module.

Doesn't that mean that standard open() should work identically?

@lathama
Copy link
Contributor Author

lathama commented Mar 5, 2017

On the IO Open vs Open, json.load changed and the io.open works the same from version to version to make sure it works with 2x and 3x Python. json.load was not in the diff so I could have called that out better.

No worries on the time frame. Just proving that it is not that hard to do. Take what you like and toss the rest. I did the proof of concept for Ansible's Python 3 transition a few years ago so sort of my thing.

I see there are other options like ports for the other protocols missing as well. I might get to that. Would be useful so that the services could be started in userland for testing as non root for the future and stuffs.

@psychomario
Copy link
Collaborator

Makes sense.

I've wanted python3 for a while now, but I've failed to complete it a few times. My main issues were with the changes in string handling, which is used a lot for the struct packing/unpacking in some of the binary modules. It didn't help that I didn't read up properly on the changes.

Some of the ports can't change, DHCP/TFTP spring to mind, but others should change easily, you're right.

@lathama
Copy link
Contributor Author

lathama commented Jun 28, 2017

Anything I should do to move this forward or should I drop this and do separate pull requests?

pypxe/server.py Outdated
print >> sys.stderr, '\nWARNING: Not root. Servers will probably fail to bind.\n'

if os.geteuid() != 0:
sys.exit("You need to have root privileges to run this script.\nPlease try again, this time using 'sudo'. Exiting.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't necessarily need to exit here, if the user is only trying to run services with non privileged ports they should still be allowed to continue, but with a warning.

In fact, more logic might be appreciated, and we only print/exit if any of the ports to be used are <1024 and we are non root

setup.py Outdated
@@ -5,11 +5,6 @@

deps = []

# Python 3 unsupported
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably only want to remove this when full Python3 support is present, so we don't get bug reports for things we know aren't working yet

@psychomario
Copy link
Collaborator

I've just added a few inline comments, and then we should be good to merge.

@psychomario
Copy link
Collaborator

Could you just squash your commits and then we should be good to go :)

@lathama
Copy link
Contributor Author

lathama commented Jun 28, 2017

Ack, adjusted PR to address those. BTW Python 3 works for me but I did only basic testing. I know there is massive room for improvement over time.

Looking at squashing via GUI as I am mobile. Does not look like there is an option in the gui.. will be a few min getting that.

@psychomario
Copy link
Collaborator

It would be handy if you could make a checklist of things that you've tested and things which are todo, so we can have an overall picture going forward.

As mentioned above I think some of the modules such as DHCP will be tricky to port because of the amount of binary data being handled.

@lathama
Copy link
Contributor Author

lathama commented Jun 28, 2017

made a mess as I was in a hurry, will sit down and attack this later after some life things

@lathama
Copy link
Contributor Author

lathama commented Jun 29, 2017

broke apart the issues into fresh PRs as I made a mess. Don't mix fast mobile Github GUI edits on master with actual work development. :(

I can close your anyone can, just leave it open until I get an ack.

@lathama
Copy link
Contributor Author

lathama commented Jun 29, 2017

all merged save for the geteuid which I will get a pr for in a few minutes

@lathama
Copy link
Contributor Author

lathama commented Jun 29, 2017

#158 catches every thing, closing this.

@lathama lathama closed this Jun 29, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants