- 
                Notifications
    
You must be signed in to change notification settings  - Fork 26
 
RSDK-12326: Job for publishing conan artifacts to server #494
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks reasonable to me! I don't have confidence personally that this is totally correct without testing, but I assume that 1) you've tested what can reasonably be tested before merge, and 2) if anything is wrong here, it's relatively harmless to just merge, run, and fix when we see the errors.
        
          
                .github/workflows/conan-publish.yml
              
                Outdated
          
        
      | c++ --version | ||
| ldd --version | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(q) curious what this is doing, if anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops leftover debugging to make sure i was getting the arch/os i wanted...joys of github actions yaml 🤡
| conan upload "viam-cpp-sdk/*" -r viamconan -c | ||
| - name: Upload additional deps | ||
| if: inputs.upload_deps && !(matrix.runner == 'buildjet-8vcpu-ubuntu-2204' && matrix.image == 'ubuntu:22.04') | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(q) curious about the runner/image constraint here, what's the reason for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's the runner/image where we get 100% cache hits from the conan center repo, so there's no need for us to duplicate their binaries in our instance
| 
           This looks great and I'm really excited for us to start using it. Am I correct to assume that we can have many different built variants of the C++ SDK cached in the repository, and we just need to start expanding the matrix?  | 
    
| 
           @acmorrow that's exactly right! hoping to make targeted expansions to the matrix (and hence the artifact cache) on an as-needed basis  | 
    
(Please consider the entire file in the PR review; it was already pushed to main to allow iterating in branches)
Introduces a new workflow for publishing binaries to the Viam Conan server. There is an additional flag for publishing the most onerous/slow deps, namely Boost and the Google dependencies.
Currently the build only focuses on one common use case for modules, namely
Further PRs should add other common configurations, such as RelWithDebInfo C++ SDK.
There is a lot of overlap between this newly created job and the Conan CI job. It remains to be seen if they could be combined, for example by adding more configurations to the build matrix, adding a workflow trigger on releases, etc.
Right now the workflow is manual only, although a release trigger would make a lot of sense. I am just thinking about how to manage automated release builds vs pushing updated versions of the slow deps to the server, which I imagine would happen much less often.