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

Removing datamodel_code_generator from main installation step #375

Merged
merged 2 commits into from
Jul 13, 2023

Conversation

aaronchongth
Copy link
Member

@aaronchongth aaronchongth commented Jul 12, 2023

Bug fix

Fixed bug

Fixes #372.

Fix applied

  • remove datamodel_code_generator from installation step as it is not released by binaries, and only emits a warning even when building rmf_api_msgs from source, not crashing the build, as per Missing dependency in building from source #249
  • datamodel_code_generator will only be used if a user is trying to create their own api-server via the schemas from rmf_api_msgs, otherwise it is not used at all
  • any mentions of datamodel_code_generator will only be in rmf_api_msgs

Verify warning instead of crash

Using podman,

podman run -it docker.io/library/ros:humble-ros-base-jammy

# inside the image
mkdir -p ~/ws/src
cd ~/ws/src
git clone https://github.com/open-rmf/rmf_api_msgs
cd ~/ws
rosdep install --from-paths src --ignore-src --rosdistro humble -y
source /opt/ros/humble/setup.bash
colcon build
# only emits warning, still builds fine

Copy link

@koonpeng koonpeng left a comment

Choose a reason for hiding this comment

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

Is there another package using datamodel_code_generator? If it is meant for rmf-web it should already be using a fixed version.

@aaronchongth
Copy link
Member Author

There is https://github.com/open-rmf/rmf_api_msgs too, I can update the readme there too.

for rmf-web, I think we are installing into the virtual environment and not system so it shouldn't matter right?

@koonpeng
Copy link

There is https://github.com/open-rmf/rmf_api_msgs too, I can update the readme there too.

for rmf-web, I think we are installing into the virtual environment and not system so it shouldn't matter right?

It half matters because rmf-web uses a venv that shares the system packages, the venv packages should have priority but in rare cases there may be problems with the transitive dependencies (actually this also applies to fastapi, uvicorn etc). I don't see any solutions though so this might be the best workaround.

After we merge this, what else do we need to do to make the build farm also use these instructions?

@aaronchongth
Copy link
Member Author

aaronchongth commented Jul 13, 2023

Right now the buildfarm doesn't care, it is actually optional https://github.com/open-rmf/rmf_api_msgs/blob/main/rmf_api_msgs/CMakeLists.txt#L95, to allow folks to use models in python (ok this is rather important still if someone wants to use binaries and python to interact with rmf-web) But right now, nothing needs to be done.

We'll need to start the conversation again, regarding migrating away from pip for anything rmf related.

koonpeng
koonpeng previously approved these changes Jul 13, 2023
Copy link

@koonpeng koonpeng left a comment

Choose a reason for hiding this comment

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

Not ideal but it's the best workaround atm.

@aaronchongth aaronchongth changed the title Using a fixed version of datamodel_code_generator and install as its own step Removing datamodel_code_generator from main installation step Jul 13, 2023
@aaronchongth aaronchongth merged commit 0ef3ebb into main Jul 13, 2023
2 checks passed
@aaronchongth aaronchongth deleted the fix/datamodel-code-generator-ver branch July 13, 2023 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants