Add regression test for visibility and more path workarounds

conda-forge / gsl-feedstock

A conda-smithy repository for gsl.

BSD 3-Clause "New" or "Revised" License

6 stars 18 forks source link

Add regression test for visibility and more path workarounds #55

Closed traversaro closed 3 years ago

traversaro commented 3 years ago

Despite local inspection, something went wrong in https://github.com/conda-forge/gsl-feedstock/pull/53, and the archives uploaded still contain the old gsl_types.h header. To debug the problem and avoid further regressions in the future, I added a compilation test for this.

Checklist

[x] Used a personal fork of the feedstock to propose changes
[x] Bumped the build number (if the version is unchanged)
[ ] Reset the build number to 0 (if the version changed)
[x] Re-rendered with the latest conda-smithy (Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
[x] Ensured the license file is being packaged.

conda-forge-linter commented 3 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

traversaro commented 3 years ago

@conda-forge-admin, please rerender

github-actions[bot] commented 3 years ago

Hi! This is the friendly automated conda-forge-webservice. I tried to rerender for you, but it looks like there was nothing to do.

traversaro commented 3 years ago

This was not expected. The regression test is working fine even without modifications, and indeed printing the headers seems to indicate that they have the correct content.

traversaro commented 3 years ago

This was not expected. The regression test is working fine even without modifications, and indeed printing the headers seems to indicate that they have the correct content.

I added more workarounds related to https://github.com/conda-forge/autotools_clang_conda-feedstock/issues/13, and I inspected the header in the test phase and the header is the expected one, and the visibility test works fine. I also added myself as a mantainer, if it is ok for you.

The PR is now ready for review @conda-forge/gsl , beside the usual drone failure.

traversaro commented 3 years ago

I also added myself as a mantainer, if it is ok for you.

If that is not ok, feel free to let me know @conda-forge/gsl, thanks!

ocefpaf commented 3 years ago

LGTM but I'd love if @isuruf could take a quick look before we merge.

isuruf commented 3 years ago

Looks good. One question. Why not do this copying in build.sh using bash instead of doing it using cmd

traversaro commented 3 years ago

Looks good. One question. Why not do this copying in build.sh using bash instead of doing it using cmd

To be honest, I did not followed all the logic path of run_autotools_clang_conda_build.bat, so I was not aware of the fact that in the end it was calling build.sh also on Windows.

traversaro commented 3 years ago

@conda-forge/gsl Just to clarify, are you happy with the PR as it is or you prefer that I move the logic of copying the header (just on Windows) in the build.sh file?

ocefpaf commented 3 years ago

@conda-forge/gsl Just to clarify, are you happy with the PR as it is or you prefer that I move the logic of copying the header (just on Windows) in the build.sh file?

@isuruf said he is fine and I don't have a strong opinion on this. Let's merge and, if we think changing that to the build.sh is better, we can do it later.