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

Utilize robotini file #23

Closed
wants to merge 3 commits into from

Conversation

kobbled
Copy link
Contributor

@kobbled kobbled commented Apr 8, 2019

Issue with running rossum for multiple robot controller version, where robot.ini file was not being taken into account. Environment variable for version was not efficient if running for multiple controller versions. Problem still arises for building same package for multiple controller with different versions, but an override argument is added to be able to control this with rossum options. I am currently just having multiple
robot.ini files for each controller and specifying them with -r ../path/to/robot.ini
during build.

kobbled added 2 commits April 7, 2019 03:18
update to python 3
Issue with running rossum for multiple robot controller version, where
robot.ini file was not being taken into account. Environment variable
for version was not efficient if running for multiple controller
version. Problem still arises for building same package for multiple
controller types, but an override arguement is added to be able to
control this with rossum options.

-parse robot.ini file with configparser and store values in tuple container
-use paths in robot.ini to find support directory, version, and base path
for ktrans.
-Error handle if paths in robot.ini do not exist, as this will cause
problems with ktransw that will not be visible in -q mode.
-add an --override argument so that users can still pick version to
ktrans to compile to.
@gavanderhoorn
Copy link
Owner

gavanderhoorn commented Apr 8, 2019

Thanks for the PR.

From a quick look it's not clear to me how this is different from using --core ID in combination with -w?

@gavanderhoorn
Copy link
Owner

One reason the support version is currently hard-coded in the build script is because it's very likely that different controller versions require different versions of libraries.

Versions of Karel < 7.70 have quite a few bits missing or functions / routines behave slightly differently or have lost/gained arguments. Versions > 8.30 have a few new routines that cannot be used on older systems.

Simply overriding the core version in a build script in those cases will typically not work, as paths to libraries have to be updated, or it might even not be needed, as different versions of projects use different workspaces altoghether.

Compare this to CMake, where regeneration of the build script is chosen over runtime adaptation of it.


Note: the above does not mean that your proposed change is not something that should be considered for inclusion into rossum. I just wanted to provide some context/rationale for why rossum behaves the way it does.

@kobbled
Copy link
Contributor Author

kobbled commented Apr 8, 2019

I agree with this, but I will try to highlight some of the issues I was having and get your thoughts:

The robot.ini file poses a lot of issues. If the paths in the robot.ini file are non existent it will not build regardless of what options are in rossum or ktransw, including the path to the roboguide robot. since ktransw is in quite mode I wanted to include error handling to catch problems with the robot.ini file in rossum, or else the user doesn't know what is going on.

@kobbled
Copy link
Contributor Author

kobbled commented Apr 8, 2019

I need to double check this to confirm that the /ver flag is ktransw is working properly. There were instances I was getting different support version files, and it was building from a different ktrans version.

@kobbled
Copy link
Contributor Author

kobbled commented Apr 8, 2019

I also wanted to get your thoughts on just using the robot.ini file for locating files and executables. The search functions you have included are nice and helpful, but if there are issues with how roboguide is installed, users will have issues. For instance I installed roboguide on a D: drive and I was running into problems pretty quick. Or just copying the winolpc bin file, support files and robot file onto a computer without roboguide installed was problematic. Forcing users to make sure there robot.ini file is correct, search problems are up to them, and it will cut down the amount of code in rossum dramatically.

@gavanderhoorn
Copy link
Owner

gavanderhoorn commented Apr 8, 2019

The robot.ini file poses a lot of issues. If the paths in the robot.ini file are non existent it will not build regardless of what options are in rossum or ktransw, including the path to the roboguide robot.

Yes, that would indeed be an option.

As I typically ignore Roboguide in my development setups, I only ever used "empty" robot.ini files, so never ran into this.

since ktransw is in quite mode I wanted to include error handling to catch problems with the robot.ini file in rossum, or else the user doesn't know what is going on.

It might be good to check if there is consistency between the info in robot.ini and what is passed to rossum (although we shouldn't lose the option of overriding that). - Edit: #24.

I need to double check this to confirm that the /ver flag is ktransw is working properly.

It should. It's not even a flag for ktransw, it's passed directly to ktrans (as it's a flag with a /).

There were instances I was getting different support version files, and it was building from a different ktrans version.

That would be a bug (or at least undesirable behaviour). If you have a reproducable example of that I would appreciate an issue over at gavanderhoorn/ktransw_py/issues.

@kobbled
Copy link
Contributor Author

kobbled commented Apr 8, 2019

ok. thank you for looking at this. I was mainly just wanting to see your thoughts on the matter. I do not expect you to merge this. I do think its a good idea to parse the robot.ini file and check the paths in the file though. Conflicts do throw errors.
I never considered just using a blank robot.ini file... I assumed that wouldn't work with how strict roboguide is about it.

@gavanderhoorn
Copy link
Owner

gavanderhoorn commented Apr 8, 2019

I also wanted to get your thoughts on just using the robot.ini file for locating files and executables.

It might make sense to integrate robot.ini better anyway, as it's going to be needed for #1 (tp files).

The search functions you have included are nice and helpful, but if there are issues with how roboguide is installed, users will have issues. For instance I installed roboguide on a D: drive and I was running into problems pretty quick. Or just copying the winolpc bin file, support files and robot file onto a computer without roboguide installed was problematic. Forcing users to make sure there robot.ini file is correct, search problems are up to them, and it will cut down the amount of code in rossum dramatically.

So I understand what you're saying, but not having Roboguide or WinOLPC installed on a machine would mean that you don't have a license to use ktrans. Or at least, not for that particular machine. I realise that ktrans works without a license (only a few registry entries are needed actually if you want to make things nice, but it works without those as well), but I'm not sure I should be facilitating that with ktransw.

Currently rossum allows you to specify the location of both ktrans (with --ktrans) and ktransw (using --ktransw) as args. You can also specify the location of the support files (using --support).

It would be a longer command line (and I haven't tested it), but I believe rossum should not start looking for Roboguide if you specify those args (or at least should not exit with an error; it if does, that would be a bug). These lines are involved in deciding whether not being able to find Roboguide should be treated as an error, or whether we can ignore that as the user has supplied the required CLAs to be able to continue.

@gavanderhoorn
Copy link
Owner

gavanderhoorn commented Apr 8, 2019

ok. thank you for looking at this. I was mainly just wanting to see your thoughts on the matter.

An issue would also have worked ;)

But I like the PR, as I can now see what you've done.

I do not expect you to merge this. I do think its a good idea to parse the robot.ini file and check the paths in the file though. Conflicts do throw errors.

Yes, that I would agree with.

I never considered just using a blank robot.ini file... I assumed that wouldn't work with how strict roboguide is about it.

Well, it's not completely empty, but almost (from here):

[WinOLPC_Util]
Robot=.

After some testing I found that for compiling Karel at least, this is what is needed to keep ktrans happy. maketp does not like it though (as I wrote in #23 (comment)).

@kobbled
Copy link
Contributor Author

kobbled commented Apr 8, 2019

Actually thinking about it rossum was never finding the roboguide reg keys on my computer. I dont know if they changed for v9 or something, but I have never looked at the output and it has found them. Want me to open an issue on this?

@gavanderhoorn
Copy link
Owner

Actually thinking about it rossum was never finding the roboguide reg keys on my computer. I dont know if they changed for v9 or something, but I have never looked at the output and it has found them. Want me to open an issue on this?

yes, please do.

@gavanderhoorn
Copy link
Owner

@kobbled: did we cover all your questions / the issues you encountered using rossum with your specific setup?

I'm not against extending / changing the way rossum finds the necessary tools (as it's far from perfect), so please open new issues if you encounter anything that you feel should behave differently.

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

Successfully merging this pull request may close these issues.

2 participants