Skip to content
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

refactor: test_package for libjpeg #23712

Merged

Conversation

ErniGH
Copy link
Contributor

@ErniGH ErniGH commented Apr 23, 2024

Simplify the test_package in according to our policies:

  • write the minimal test that tests the package (the compiler can find the correct includes, the static linker can resolve symbols, the runtime linker can resolve symbols)
  • do not perform heavy logic in CI
  • remove binary blob - these will no longer be allowed anywhere in the repository
  • Remove the test_v1_package and the image - it is not worth backporting the refactor to the legacy test

@CLAassistant
Copy link

CLAassistant commented Apr 23, 2024

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@ghost
Copy link

ghost commented Apr 23, 2024

I detected other pull requests that are modifying libjpeg/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@valgur valgur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's ok to remove test_v1_packages already, but otherwise looks good. Thanks! The test executable was doing way too much, indeed.

@@ -10,11 +10,3 @@ if (MSVC)
target_compile_definitions(${PROJECT_NAME} PRIVATE _CRT_SECURE_NO_WARNINGS _CRT_NONSTDC_NO_WARNINGS)
endif()

if(BUILD_TRANSUPP)
add_executable(test_transupp test_transupp.c ${LIBJPEG_RES_DIR}/transupp.c)
target_link_libraries(test_transupp PRIVATE JPEG::JPEG)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably still test this minimally - this appears to be unconditionally included in the recipe.
perhaps just add an #include "transupp.h" in test_package.c and that's it.

Copy link
Contributor

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small comment, but rest is great :D

@jcar87
Copy link
Contributor

jcar87 commented Apr 23, 2024

I'm not sure if it's ok to remove test_v1_packages already, but otherwise looks good. Thanks! The test executable was doing way too much, indeed.

Thanks @valgur!

One of the goals of this PR is not only to simplify the test package, but to also remove any test that make use of binary files - these will no longer be accepted in any test package or in any PR. test_v1_package are no longer required for newly added recipes (as per announcement) - but they can be removed if they get in the way of maintenance - in this case I see no point in backporting the simplification to the old test package. Perhaps we can word this better in the announcement. What we won't accept and is only reserved to Conan team maintainers, is removal of test_v1_package without any reason - the team will deal with the removal at a later date (@ErniGH is a member of the team).

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 5 (080e2671a8590a4a17a8c87256c20ec6d97f246f):

  • libjpeg/9e:
    All packages built successfully! (All logs)

  • libjpeg/9c:
    All packages built successfully! (All logs)

  • libjpeg/9d:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 5 (080e2671a8590a4a17a8c87256c20ec6d97f246f):

  • libjpeg/9e:
    All packages built successfully! (All logs)

  • libjpeg/9d:
    All packages built successfully! (All logs)

  • libjpeg/9c:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit f49dd18 into conan-io:master Apr 23, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants