Skip to content

Conversation

@DLu
Copy link
Contributor

@DLu DLu commented Sep 29, 2018

This PR has not been tested in isolation, but is seemingly valid Python.

This is mostly to debate whether or not I'm allowed to use Python requests. The conversation was previously here where @nuclearsandwich said they were warming to the new dependency, but I wanted to reopen that conversation.

@nuclearsandwich
Copy link
Contributor

This is mostly to debate whether or not I'm allowed to use Python requests.

Short answer: Green light. Please do some Trusty testing.

Rationale: Requests is available via system package managers in every platform supported by rosdep and Alpine where rosdep support is still in progress. bloom itself doesn't leverage the rosdep database for its dependencies but it's a good enumeration of ROS's possible target platforms.
My primary concern with adding dependencies to ROS infrastructure packages is that it increases the theoretical overhead of porting ROS to a new platform. Bloom is a bit of a special case in that regard as it is run primarily on developer workstations rather than as part of a "live" ROS installation. Additionally, Requests is well tested, widely used, and available via pip which is how most new ROS platforms will likely start building their ROS toolchain, including bloom.

Please be sure to set the requests minimum version to 2.2 in setup.py to signify that it was the oldest tested and update the stdeb.cfg dependencies.

@DLu
Copy link
Contributor Author

DLu commented Oct 11, 2018

I still need to do the requested Trusty testing on this, but please check that I added the dependencies properly.

@DLu
Copy link
Contributor Author

DLu commented Nov 5, 2018

@nuclearsandwich I finally had some time to install trusty and check things here. requests >= 2.2 is valid there, and the CI is now passing.

@DLu DLu mentioned this pull request Dec 4, 2018
@nuclearsandwich
Copy link
Contributor

@DLu I know I mentioned privately that I had no reservations about this PR. And that's still largely the case, but what was the reason for removing the requests module from setup.py? I believe it will still be needed there for when bloom is installed via pip / from source.

@DLu
Copy link
Contributor Author

DLu commented Jan 11, 2019

Sorry for the delay.

I'm not sure how setup.py really works, but with requests module in setup.py, it broke the CI

Without it, it passes.

🤷‍♂️

@nuclearsandwich
Copy link
Contributor

That's something I'll have to investigate then. I'm pretty certain it needs to be there for users who install bloom via pip.

@DLu
Copy link
Contributor Author

DLu commented Dec 13, 2019

@nuclearsandwich Is this something there is still interest in merging in?

@DLu
Copy link
Contributor Author

DLu commented Jun 10, 2020

With the advent of Tailor I don't need this anymore, but I've updated it if others think there is sufficient use for Gitlab.

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