-
Notifications
You must be signed in to change notification settings - Fork 34
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
ElasticLib CI #210
base: main
Are you sure you want to change the base?
ElasticLib CI #210
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #210 +/- ##
==========================================
- Coverage 75.96% 75.89% -0.08%
==========================================
Files 79 79
Lines 8618 8626 +8
==========================================
Hits 6547 6547
- Misses 2071 2079 +8 ☔ View full report in Codecov by Sentry. |
These CI workflows are kinda cursed, tbh. An actual vendordep will download and link the dependencies for you. You could use the files from https://github.com/wpilibsuite/vendor-template/. If those don't work, https://github.com/SleipnirGroup/Choreo/tree/main/choreolib is a working reference. |
Ye ik it's pretty cursed, I'll try and set this up normally |
I do agree with Tyler that this is a cursed way to do this, but I'm also not super into the idea of bundling this as a vendordep. There's 2 reasons I haven't really pursued doing it in the past:
ElasticLib is just 1 file (2 if you're using C++), so there's not really a need to check for build errors every time. From what I've heard though, the process for setting up a vendordep doesn't have a lot of documentation outside of the vendordep template
Releasing an entire vendordep separate from Elastic would be a lot more difficult, as those would also have to be synced up to WPILib releases. Releasing Elastic in parallel with WPILib isn't too bad, I just have to make sure to release Elastic a day or two before WPILib and update the tag the installer uses, but using a vendordep would mean that it also has to be built against a specific WPILib version, which would make things much more complicated. This was an issue that Choreo had when releasing the vendordep alongside the app in a WPILib release If ElasticLib grows into a multi-file library, then this is definitely something that could be considered, but for just 1 or 2 files, I think the "copy this file" method works just fine |
That was Choreo's experience. We had to make modifications to the vendor template for our needs. We also had to ask Thad how to fix a few issues we had.
You don't have to actually publish the vendordep. You could just use the vendordep infrastructure in CI to confirm elasticlib compiles, without the error-prone manual dependency downloading. If CI-with-vendordep has WPILib version compat issues (which I don't think it does), then CI-with-manual-downloads would have the same compat issues, cuz it's the same libraries and linker flags either way. Vendordeps need to recompile against WPILib when upstream ABI breaks occur, but those don't happen mid-season. |
|
||
- name: Compile C++ Code | ||
run: | | ||
g++ -std=c++17 \ |
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.
FYI, WPILib compiles with C++20, not C++17.
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.
This might be why i didnt manage for it to compile
This doesnt work for C++, I dont know what is the right download for the units library should be and thus getting a linker error (tried some stuff as visible), works for Java build