G-Research / ParquetSharp

ParquetSharp is a .NET library for reading and writing Apache Parquet files.
Apache License 2.0
183 stars 49 forks source link

[CI] Fix deprecation warnings #309

Closed pavlovic-ivan closed 1 year ago

pavlovic-ivan commented 2 years ago

update the ParquetSharp CI to avoid warnings and errors like the ones we get today? https://github.com/G-Research/ParquetSharp/actions/runs/3250581963

quick assessment is:

Similar PR: https://github.com/m4rs-mt/ILGPU/pull/866

jgiannuzzi commented 2 years ago

@adamreeve has created #310 in the meantime — the only missing change is the ubuntu-20.04 update.

jgiannuzzi commented 2 years ago

We should also consider using Dependabot for actions only — it would have avoided most of these issues in the first place.

pavlovic-ivan commented 1 year ago

hey @jgiannuzzi. where are we with this issue? do we still need to contribute or not? did Adam with his PR resolve the issues?

adamreeve commented 1 year ago

We still need to upgrade the Ubuntu version, and I noticed we're now getting a new warning: The target framework 'net5.0' is out of support and will not receive security updates in the future So we should probably drop net 5.0 and add .NET 7 to the test matrix.

Using Dependabot for actions would be helpful. We have an old PR (#212) open to add Dependabot but covering NuGet packages too. I think there were questions around whether it makes sense to use Dependabot for library dependencies?

pavlovic-ivan commented 1 year ago

thanks @adamreeve i will take a look

adamreeve commented 1 year ago

@ljubon has been working on migrating the GitHub actions Linux runner to Ubuntu 22.04 but has hit an issue with the Linux ARM64 tests not working, they all throw exceptions like:

System.DllNotFoundException : Unable to load shared library 'ParquetSharpNative' or one of its dependencies. In order to help diagnose loading problems, consider setting the LD_DEBUG environment variable: libParquetSharpNative: cannot open shared object file: No such file or directory

This should be fixed before we go ahead with moving to 22.04. One relevant change is that installing the pkg-config-aarch64-linux-gnu package was removed as this is no longer present in the Ubuntu 22.04 repositories (it is still in 20.04 though). I doubt that this explains the failure though as the build on arm64 appears to work fine and the arm .so is present in the NuGet package, it's only the tests that fail.

I wonder if the problem is just that we're now building with a newer version of Ubuntu than we test with, so the built dll is incompatible with the older Ubuntu version (eg. due to depending on a newer glibc than what's available). If that's the case we should probably stick with building on 20.04 rather than moving to 22.04, unless we're happy to require Ubuntu 22.04 for the Arm package. We have a similar issue with the x64 builds I believe, and work around this by installing a developer toolset from a CentOS docker container.

ljubon commented 1 year ago

Hi @adamreeve here is the latest report on tests: :red_circle: running the arm tests on 22.04 - https://github.com/ljubon/ParquetSharp/actions/runs/4168370029/jobs/7215229057#logs :green_circle: building and testing on 20.04 (kept 22.04, just added Build native arm64 library (ubuntu-20.04) support) - https://github.com/ljubon/ParquetSharp/actions/runs/4269258266

Let me know if it's ok to have build-native step done on both 20.04 and 22.04, only with this scenario all tests are passing

Here is the internal PR: https://github.com/ljubon/ParquetSharp/pull/77

adamreeve commented 1 year ago

Hi @ljubon, no, we shouldn't be building the arm64 native library on both 20.04 and 22.04. When we build the NuGet package we take a single arm64 library from the arm64-linux-native-library artifact. As both the 22.04 and 20.04 builds are creating this artifact I assume it must be overwritten by the 20.04 build which is allowing the tests to pass. I'm confused why you say that only in this scenario all tests are passing though as it shouldn't be possible for the tests to depend on both the builds, there's only a single ParquetSharpNative.so file in the artifact.

I was a bit surprised that building on 22.04 and then testing on 22.04 didn't work, but then I saw that we run the arm tests in a Docker container using the mcr.microsoft.com/dotnet/sdk:6.0 image, which is based on Debian 11. Ubuntu 20.04 appears to be Debian 11 based, whereas Ubuntu 22.04 is Debian 12 based, so this makes sense. I think it's fine to just keep testing on 20.04 for now.

jgiannuzzi commented 1 year ago

That’s it, there are no ubuntu-18.04 runners anymore! It would be great to push this one over the line as our CI is now essentially broken. Upgrading to ubuntu-20.04 would be enough given the urgency.

pavlovic-ivan commented 1 year ago

@jgiannuzzi @adamreeve Ljubo and I are on it. We will first focus on fixing the broken CI.

ljubon commented 1 year ago

For now, PR #346 is opened to address this urgent issue The bigger change, with adding .net7.0 and removing .net5.0 would come after we are back to 🟢 with pipelines

pavlovic-ivan commented 1 year ago

@jgiannuzzi @adamreeve Ljubo and i have agreed to make the CI green again, just by bumping bumping/removing ubuntu-18.04 runners. This is done with the PR Ljubo made above, but it's a separate branch, and an independent PR. The other one, that is related to this issue from the beginning, is still in progress. I would ask if you @adamreeve and @ljubon to sync on what is the best course of action to move the older PR further. I've seen @adamreeve had some remarks as well. Let's get this sorted out. Feel free to include me in a group thread if that is where you would like to discuss this further

ljubon commented 1 year ago

Merged: https://github.com/G-Research/ParquetSharp/pull/363