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

Grasshopper GHPY libraries install #770

Merged
merged 11 commits into from
Feb 12, 2021
Merged

Conversation

romanarust
Copy link
Member

@romanarust romanarust commented Feb 12, 2021

On COMPAS FAB we started to make Grasshopper components that are compiled in a library (.GHPY) (compas-dev/compas_fab#249).
For installing that library, it needs to be copied into the Grasshopper library path. This action is ideally executed when calling
python -m compas_rhino.install

As suggested by @gonzalocasas, compas core should know how to install GH components, and other packages only need to provide the component locations.

For the time being, we're providing two extension points (pluggable interfaces) to allow hooking up into the install/uninstall and do arbitrary stuff. This will be a good solution until we sort out all the details of the installation (and potentially compilation) of GH components on end-user machines.

What type of change is this?

  • Bug fix in a backwards-compatible manner.
  • New feature in a backwards-compatible manner.
  • Breaking change: bug fix or new feature that involve incompatible API changes.
  • Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
  • I ran all tests on my computer and it's all green (i.e. invoke test).
  • I ran lint on my computer and there are no errors (i.e. invoke lint).
  • I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

@gonzalocasas
Copy link
Member

@romanarust I made the method public, because we'll be using it from compas_fab already now, so if it's public it would be clearer if someone at some point wants to remove/refactor it, that there might be other code using it.

@gonzalocasas gonzalocasas marked this pull request as ready for review February 12, 2021 16:08
Copy link
Member

@beverlylytle beverlylytle left a comment

Choose a reason for hiding this comment

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

The Extension Points documentation additions don't link anywhere, but that can be taken later. Code looks good, but I admit there's been no functional testing.

for item in result:
try:
package, message, success = item
status = 'OK' if success else 'ERROR'
Copy link
Member

Choose a reason for hiding this comment

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

should an error here result in an exit_code = -1?

Copy link
Member

Choose a reason for hiding this comment

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

you're right, fixed at 7b6ef0d

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a little more complicated. Shouldn't all the successes be collected into some list successes and then instead of returning True, all(successes) is returned?

@gonzalocasas
Copy link
Member

just as fyi, this is how the thing runs with a simple (fake) implementation of the pluggable:

image

status = 'OK' if success else 'ERROR'
print(' {} {}: {}'.format(package.ljust(20), status, message))
except ValueError:
post_execution_errors.append(ValueError('Step successful, but result is wrongly formatted: {}'.format(str(result))))
Copy link
Member

Choose a reason for hiding this comment

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

and here, should result be item?

Copy link
Member

Choose a reason for hiding this comment

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

ups! 6440b4d

for error in post_execution_errors:
print(' - {}'.format(repr(error)))

return False
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing to me. If there are post_execution_errors, then it's been printed that the step was successful, but now we are getting an -1 exit code. So is it a success or an error?

Copy link
Member

Choose a reason for hiding this comment

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

How about now? I fixed the issue you found here and hopefully made it a bit clearer what the return value of this method is supposed to be: ae01ee4

Copy link
Member

Choose a reason for hiding this comment

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

I guess it's that I don't like the message of the ValueError. If the result isn't formatted correctly how can it be assume that the step was successful?

Copy link
Member

Choose a reason for hiding this comment

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

ahh... mmm... fair point. how about: ValueError('Step ran without errors but result is wrongly formatted: {}'.format(str(item))?

Copy link
Member

@beverlylytle beverlylytle Feb 12, 2021

Choose a reason for hiding this comment

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

that is a less confusing message for me.

Copy link
Member

Choose a reason for hiding this comment

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

done! 4836330

@tomvanmele tomvanmele merged commit f4cb6fe into main Feb 12, 2021
@tomvanmele tomvanmele deleted the add-grasshopper_library_path branch March 14, 2021 10:54
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.

4 participants