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.

Sign up for GitHub

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

Jump to bottom

Fix source build instructions for ign-gazebo3 #395

Merged
merged 3 commits into from
Oct 8, 2020
Merged

Fix source build instructions for ign-gazebo3 #395

merged 3 commits into from
Oct 8, 2020

Conversation

adlarkin
Copy link
Contributor

@adlarkin adlarkin commented Oct 7, 2020

When trying to build ign-gazebo3 from source, @chapulina and I noticed a few things:

  1. The qtdeclarative5-models-plugin third-party dependency seems unnecessary (this package also doesn't exist for Ubuntu Focal - only exists for Ubuntu Bionic)
  2. There were missing dependencies for ign-sensors and ign-fuel-tools

Users should now be able to go through the source build instructions without any issues.

Signed-off-by: Ashton Larkin [email protected]

@adlarkin adlarkin requested a review from chapulina October 7, 2020 02:11
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Oct 7, 2020
@adlarkin
Copy link
Contributor Author

adlarkin commented Oct 7, 2020

Another thing to note is that the source install link in the README is currently broken because the section it links to is labeled as Source Install (version 3) instead of Source Install. Should we strip the (version 3) from this header to fix this link?

chapulina
chapulina reviewed Oct 7, 2020
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Should we strip the (version 3)

Yeah I think that makes it simpler

README.md Outdated
@@ -140,7 +140,7 @@ for dependency installation instructions for each supported operating system.
1. Install third-party libraries:

```
sudo apt-get -y install cmake build-essential curl cppcheck g++-8 libbenchmark-dev libgflags-dev doxygen ruby-ronn libtinyxml2-dev libtinyxml-dev software-properties-common libeigen3-dev qtdeclarative5-models-plugin
sudo apt-get -y install cmake build-essential curl cppcheck g++-8 libbenchmark-dev libgflags-dev doxygen ruby-ronn libtinyxml2-dev libtinyxml-dev software-properties-common libeigen3-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that every repository has all the dependencies listed in .github/ci/packages.apt, we should start using that in the installation instructions so the instructions are always up-to-date. What do you think of doing that here?

For example, running this after cloning:

export SYSTEM_VERSION=bionic
sudo apt -y install \
  $(sort -u $(find . -iname 'packages-'$SYSTEM_VERSION'.apt' -o -iname 'packages.apt') | tr '\n' ' ')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is definitely a cleaner approach that is easier to maintain. I went ahead and made this change in a18718a.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Just note that the user can only run that after cloning the repository, so that instruction should be moved down, or the cloning moved up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just noticed that as well. I went ahead and made a note about cloning first in 4ae6846.

@adlarkin
Copy link
Contributor Author

adlarkin commented Oct 7, 2020

Another thing to note is that the source install link in the README is currently broken because the section it links to is labeled as Source Install (version 3) instead of Source Install. Should we strip the (version 3) from this header to fix this link?

@chapulina I have also noticed that the Binary Install instructions install ign-gazebo2, not ign-gazebo3. I'm assuming that this was written at a time when the debian package for ign-gazebo3 was not yet available - since it's available now, should I fix the binary install instructions to download ign-gazebo3?

@chapulina
Copy link
Contributor

should I fix the binary install instructions to download ign-gazebo3?

Yes, please, thanks. The ign-gazebo3 branch should direct the user to install that version. We should also check that ign-gazebo2 and ign-gazebo4 are correct.

@adlarkin adlarkin requested a review from chapulina October 8, 2020 00:02
@adlarkin
Copy link
Contributor Author

adlarkin commented Oct 8, 2020

We should also check that ign-gazebo2 and ign-gazebo4 are correct.

It looks like ign-gazebo2 and ign-gazebo4 are correct, but they don't make use of the packages*.apt files in the source install instructions - would you like me to update the READMEs for those branches to use packages*.apt as well?

@chapulina
Copy link
Contributor

would you like me to update the READMEs for those branches to use packages*.apt as well?

That would be awesome, thanks!

@chapulina chapulina merged commit 9e7f6ae into ign-gazebo3 Oct 8, 2020
@chapulina chapulina deleted the adlarkin/source_build_fix branch October 8, 2020 17:33
@adlarkin
Copy link
Contributor Author

adlarkin commented Oct 9, 2020

I know this is already merged, but one other thing I just noticed is that we don't mention installing cppcheck in order to run make codecheck (https://github.com/ignitionrobotics/ign-gazebo/tree/ign-gazebo3#testing). Should we add this to the README? If so, should I just make another PR for this?

This was referenced Oct 12, 2020
Merged
Merged
doisyg pushed a commit to wyca-robotics/ign-gazebo that referenced this pull request Dec 13, 2020
@adlarkin
Fix source build instructions for ign-gazebo3 (gazebosim#395)
37af27d 
* using .apt files to get source build dependencies

Signed-off-by: Ashton Larkin <[email protected]>
Signed-off-by: Guillaume Doisy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

Assignees
No one assigned
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adlarkin @chapulina