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

Allow settings_target across compatibility method #14532

Conversation

adnan-ali1
Copy link
Contributor

@adnan-ali1 adnan-ali1 commented Aug 21, 2023

Changelog: Feature: Allow access to settings_target in compatibility method.
Docs: Omit

Closes #14527

In the info.py module, following the guidance provided in the comments for the settings_target property, this commit focuses on refining the handling of settings_target within various package methods.

When populating the settings_target property via the package_id() method, the adjustment guarantees that it remains accessible to both the info and original_info methods. By extending its accessibility to these methods, it allows for seamless utilization within the compatibility() method as well.

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Test Case Result

Before

E
E           ======== Computing necessary packages ========
E           tool/0.1: Compatible package ID 30b28f4b57d074979ce37758614b0bd2ca007f31 equal to the default package ID
E           app/0.1: Forced build from source
E           Requirements
E               app/0.1#0a0d67a98e79b54a848dc56809f632a4:4acd47797ab70f037765f4aa81d97e141228872f - Build
E           Build requirements
E               tool/0.1#643d7250bb4082abb8ef774ceb2e720c:30b28f4b57d074979ce37758614b0bd2ca007f31 - Missing
E
E           ======== Installing packages ========
E           ERROR: Missing binary: tool/0.1:30b28f4b57d074979ce37758614b0bd2ca007f31
E
E           tool/0.1: WARN: Can't find a 'tool/0.1' package binary '30b28f4b57d074979ce37758614b0bd2ca007f31' for the configuration:
E           [settings]
E           arch=x86_64
E           [settings_target]
E           arch=armv7

After

====================================== test session starts ===========================
platform linux -- Python 3.10.6, pytest-6.2.5, py-1.11.0, pluggy-1.2.0
rootdir: /mnt/c/Users/AAli/Desktop/workspace/conan, configfile: pytest.ini
plugins: xdist-3.3.1
collected 1 item

conans/test/integration/package_id/test_validate.py .                                                                                                                                                       [100%]

====================================== 1 passed in 1.14s ==============================

@CLAassistant
Copy link

CLAassistant commented Aug 21, 2023

CLA assistant check
All committers have signed the CLA.

In the info.py module, following the guidance provided in the comments
for the settings_target property, this commit focuses on refining the
handling of settings_target within various package methods.

When populating the settings_target property via the package_id()
method, the adjustment guarantees that it remains accessible to both the
info and original_info methods. By extending its accessibility to these
methods, it allows for seamless utilization within the compatibility()
method as well.

Signed-off-by: Adnan Ali <m_adnanali_1@hotmail.com>
@adnan-ali1 adnan-ali1 force-pushed the feature/14527-settings_target_in_compatibility branch from f46ad31 to 1a20b3b Compare August 21, 2023 11:43
Signed-off-by: Adnan Ali <m_adnanali_1@hotmail.com>
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Thanks very much for the contribution @adnan-ali1

I think this is looking good overall, but it might still need some improvement and some other extra tests, please let us know if the comments are enough or you'd like some help with that.

conans/client/graph/compute_pid.py Outdated Show resolved Hide resolved
conans/client/graph/compatibility.py Show resolved Hide resolved
@memsharded memsharded added this to the 2.0.11 milestone Aug 25, 2023
@adnan-ali1
Copy link
Contributor Author

@memsharded thanks for the review. I will update the PR with the proposed changes

Prevents the crash as in case a requirements is not
build as a build_require it does not even has a settings_target
and settings some property in null object would crash.

Update test cases to check for multiple scenerios:

Signed-off-by: Adnan Ali <m_adnanali_1@hotmail.com>
@adnan-ali1 adnan-ali1 requested a review from memsharded August 30, 2023 21:29
take in the suggestion.

Co-authored-by: James <memsharded@gmail.com>
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Changes are good, and tests are clean and comprehensive, great job, thanks very much!

Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

@adnan-ali1 thanks a lot for your contribution, it's really appreciated!

I like the simplicity of adding this feature and the tests you provided are quite comprehensive :)

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.

[feature][2.0.9] settings_target accessible in the compatibility method
4 participants