Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

build: add DOWNLOAD_ARTIFACTS option to let user choose. Defaults to false #164

Closed
wants to merge 1 commit into from
Closed

build: add DOWNLOAD_ARTIFACTS option to let user choose. Defaults to false #164

wants to merge 1 commit into from

Conversation

esteve
Copy link
Contributor

@esteve esteve commented Mar 22, 2023

Description

Added option for all packages to let user choose if they want to download artifacts. Defaults to false.

See https://gitlab.com/autowarefoundation/autoware.auto/AutowareAuto/-/blob/15d2e91a2b3c8cd61b20b7e6e79da32919c2cbcd/src/tools/autoware_auto_cmake/cmake/autoware_auto_cmake.cmake#L33)

Fixes autowarefoundation/autoware.universe#3137

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (b30849e) 11.23% compared to head (66c57e8) 11.23%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #164   +/-   ##
=======================================
  Coverage   11.23%   11.23%           
=======================================
  Files          33       33           
  Lines        2385     2385           
  Branches     1260     1260           
=======================================
  Hits          268      268           
  Misses       1672     1672           
  Partials      445      445           
Flag Coverage Δ *Carryforward flag
differential 11.23% <ø> (?)
total 11.23% <ø> (ø) Carriedforward from b30849e

*This pull request uses carry forward flags. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mitsudome-r
Copy link
Member

I have a question regarding adding the build option. When I run the build, there are a lot of warning message coming out from the packages that does not use DOWNLOAD_ARTIFACTS variable. Are there anyway to suppress these warnings other than filtering the packages by --package-up-to?

build command:

colcon build --symlink-install --cmake-args -DCMAKE_BUILD_TYPE=Release -DDOWNLOAD_ARTIFACTS=true

warning message:

--- stderr: autoware_auto_system_msgs
CMake Warning:
  Manually-specified variables were not used by the project:

    DOWNLOAD_ARTIFACTS
---

@esteve
Copy link
Contributor Author

esteve commented Mar 23, 2023

@mitsudome-r thanks for trying it out. Yes, that message can be annoying, let me check if there's a way to suppress it.

@esteve
Copy link
Contributor Author

esteve commented Mar 23, 2023

@mitsudome-r I tried a few things, but they won't work because not all packages depend on autoware_cmake. The reason for this warning is that some Autoware packages (e.g. autoware_lint_common) and especially, external packages (e.g. ndt_omp), don't depend on autoware_cmake and therefore don't use autoware_package, which is where the option is handled. So if DOWNLOAD_ARTIFACTS is not read anywhere (as it happens in packages that don't use autoware_package), CMake outputs that warning.

So the options I can think of are:

  • convert everything to autoware_cmake, which would have the benefit of making packages more uniform, but require some work
  • handle DOWNLOAD_ARTIFACTS in each separate package, as we do now

The reason it worked with https://gitlab.com/autowarefoundation/autoware.auto/AutowareAuto/-/blob/15d2e91a2b3c8cd61b20b7e6e79da32919c2cbcd/src/tools/autoware_auto_cmake/cmake/autoware_auto_cmake.cmake#L33 is because all the Autoware.Auto packages used autoware_auto_cmake

@esteve
Copy link
Contributor Author

esteve commented Mar 24, 2023

I'm closing this PR because following the discussion in autowarefoundation/autoware.universe#3137 (comment) we will remove the logic for downloading artifacts, and deploy them via ansible and point users where to manually download them.

@esteve esteve closed this Mar 24, 2023
@esteve esteve deleted the add-download-artifacts-option branch March 24, 2023 13:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move downloading artifacts outside CMake
2 participants