Hi Brad, Great! Thanks a lot! It looks almost good to me.
A few remarks, if update sys.path before importing module file, I guess we need to backup the original sys.path and restore it before returning module instance as following. https://github.com/osrg/ryu/blob/master/ryu/utils.py#L96-L97 This avoids inserting the unexpected path into sys.path. Thanks, Iwase On 2017年04月08日 12:25, Brad Cowie wrote: > Hi Iwase, > > Thanks for the followup. > > In testing I found a few issues with your patch and have some feedback: > > - Windows style line endings > - Breaks python2.6 & python2.7 compatibility (FileNotFoundError is python3 > only) > - Doesn't update sys.path to include path to python file which causes issues > with imports > > I have written an updated patch and attached that fixes these issues and > further reworks the loading behaviour of Ryu. > > What I've done is switched from using exceptions to an if statement to decide > on how a given RyuApp should be loaded. The reason I've done this is because > imp.load_source and importlib will pass exceptions from the user code up the > stack to Ryu which means that an exception inside the RyuApp would cause us > to do a module import even though we already loaded the RyuApp from a file. > > Let me know what you think of this patch. > > Brad > > On 7 April 2017 at 19:56, Iwase Yusuke <iwase.yusu...@gmail.com > <mailto:iwase.yusu...@gmail.com>> wrote: > > Hi Brad, > > Sorry for the delay. > > I make a patch for this issue. > Could you test the attached patch? > > Thanks, > Iwase > > > On 2017年03月24日 17:19, Brad Cowie wrote: > > Hi Iwase, > > > > Yes I suspect most people will prefer that when a file path is > specified as a ryu app that file path given is preferred over loading a > module. > > > > Perhaps an additional check - if the app given to ryu-manager exists as > a regular file, load that, otherwise load the module by the same name? > > > > Brad > > > > On 24 March 2017 at 19:46, Iwase Yusuke <iwase.yusu...@gmail.com > <mailto:iwase.yusu...@gmail.com> <mailto:iwase.yusu...@gmail.com > <mailto:iwase.yusu...@gmail.com>>> wrote: > > > > Hi Brad, > > > > Thank you for your reporting! > > > > Yes, as you said, ryu.utils.import_module() prefers to loading a > Python module than file paths. > > In order to solve this problem, we need to change the priority > (Python module or file path), I guess. > > > > You mean "file path should be preferred", right? > > > > Thanks, > > Iwase > > > > > > On 2017年03月19日 11:05, Brad Cowie wrote: > > > Hi there, > > > > > > If you have a RyuApp and put it in a python file that happens to > have the same name as any python module installed in PYTHON_PATH, ryu-manager > will be unable to load the RyuApp. > > > > > > Here is an example showing the issue in ryu-manager 4.12: > > > > > > brad@zilch ~/D/r/r/app> ryu-manager ./simple_switch_13.py > > > loading app ./simple_switch_13.py > > > loading app ryu.controller.ofp_handler > > > instantiating app ./simple_switch_13.py of SimpleSwitch13 > > > instantiating app ryu.controller.ofp_handler of OFPHandler > > > [loads fine] > > > > > > brad@zilch ~/D/r/r/app> pip freeze | grep six > > > six==1.10.0 > > > [find name of any package installed with pip] > > > > > > brad@zilch ~/D/r/r/app> mv ./simple_switch_14.py > ./six.py > > > brad@zilch ~/D/r/r/app> ryu-manager ./six.py > > > loading app ./six.py > > > [ryu-manager will attempt to load a RyuApp from > /usr/lib/python2.7/dist-packages/six.py instead of ./six.py] > > > > > > This seems to be an issue in ryu > <https://github.com/osrg/ryu/tree/master/ryu > <https://github.com/osrg/ryu/tree/master/ryu> > <https://github.com/osrg/ryu/tree/master/ryu > <https://github.com/osrg/ryu/tree/master/ryu>>>/*utils.py* where > import_module() will prefer to load a python module even when presented with > a relative or absolute file path. > > > > > > Interested on people's thoughts on the best way to solve this one. > > > > > > Regards, > > > Brad > > > > > > > > > > ------------------------------------------------------------------------------ > > > Check out the vibrant tech community on one of the world's most > > > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > > > > > > > > > > > > _______________________________________________ > > > Ryu-devel mailing list > > > Ryu-devel@lists.sourceforge.net > <mailto:Ryu-devel@lists.sourceforge.net> > <mailto:Ryu-devel@lists.sourceforge.net > <mailto:Ryu-devel@lists.sourceforge.net>> > > > https://lists.sourceforge.net/lists/listinfo/ryu-devel > <https://lists.sourceforge.net/lists/listinfo/ryu-devel> > <https://lists.sourceforge.net/lists/listinfo/ryu-devel > <https://lists.sourceforge.net/lists/listinfo/ryu-devel>> > > > > > > > > > > > > > > ------------------------------------------------------------------------------ > > Check out the vibrant tech community on one of the world's most > > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > > > > > > > > _______________________________________________ > > Ryu-devel mailing list > > Ryu-devel@lists.sourceforge.net <mailto:Ryu-devel@lists.sourceforge.net> > > https://lists.sourceforge.net/lists/listinfo/ryu-devel > <https://lists.sourceforge.net/lists/listinfo/ryu-devel> > > > > > > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > > > > _______________________________________________ > Ryu-devel mailing list > Ryu-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/ryu-devel > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Ryu-devel mailing list Ryu-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ryu-devel
