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.

Sign up for GitHub

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

Jump to bottom

Feature/Add an option to install() method in package_manager #14752

Merged
merged 6 commits into from
Sep 25, 2023
Merged

Feature/Add an option to install() method in package_manager #14752

merged 6 commits into from
Sep 25, 2023

Conversation

LielHarel
Copy link
Contributor

@LielHarel LielHarel commented Sep 15, 2023 •
edited by czoido
Loading

Changelog: Feature: Add host_tool to install() method in package_manager to indicate whether the package is a host tool or a library.
Docs: conan-io/docs#3401

According to that the we will install a package for the needed arch (host arch or destination arch which can be different in case of cross building)

This option enables the user controls whether the installed package is for the host or needed for the destination arch, this is useful specifically when cross building and we want to avoid misunderstanding about what we want to install.

Closes #14567

  • 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.

@LielHarel LielHarel changed the title Add a qualifier to install() method in package_manager that Feature/Add a option to install() method in package_manager Sep 15, 2023
@LielHarel LielHarel changed the title Feature/Add a option to install() method in package_manager Feature/Add an option to install() method in package_manager Sep 15, 2023
@LielHarel LielHarel mentioned this pull request Sep 15, 2023
Closed
1 task
@CLAassistant
Copy link

CLAassistant commented Sep 15, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

memsharded
memsharded reviewed Sep 15, 2023
View reviewed changes
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 for contributing to Conan!

conan/tools/system/package_manager.py Outdated Show resolved Hide resolved
memsharded
memsharded reviewed Sep 15, 2023
View reviewed changes
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.

Some kind of testing to cover this would be necessary. Likely to be unit-test with some patching/mocking, not easy for a full test with this. Please have a look, don't hesitate to ask for help, we might even be able to contribute the tests ourselves.

@LielHarel LielHarel mentioned this pull request Sep 15, 2023
Closed
memsharded
memsharded reviewed Sep 16, 2023
View reviewed changes
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.

I think we are almost there, thanks for your work! 🙂

I am concerned that this change might be breaking existing usage, please have a look to the comments.
When the existing tests doesn't need to be changed, that is a very good sign, then adding a new test, that checks the other non-default new behavior. Please let me know if you need with that new test.

conan/tools/system/package_manager.py Outdated Show resolved Hide resolved
@memsharded memsharded added this to the 2.0.12 milestone Sep 17, 2023
AbrilRBS
AbrilRBS approved these changes Sep 17, 2023
View reviewed changes
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.

Thanks for taking the opportunity to contribute the fix by yourself, we really appreciate it :)

@AbrilRBS AbrilRBS linked an issue Sep 17, 2023 that may be closed by this pull request
[bug] Install packages compatible with incorrect architecture #14567
Closed
1 task
@AbrilRBS AbrilRBS changed the base branch from develop to release/2.0 September 17, 2023 23:34
@AbrilRBS
Copy link
Member

Thanks! I've rebased this against the release/2.0 branch - I'll fix the conflicts tomorrow :)

@AbrilRBS AbrilRBS self-assigned this Sep 17, 2023
czoido and others added 2 commits September 18, 2023 14:42
@czoido @LielHarel
Fix scons test and conf (conan-io#14729)
0841363 
correct scons test and conf
@LielHarel
- Add a qualifier to install() method in package_manager that
a22374e 
indicates whether the package is for the host machine (that will run the software)
or for the build machine.
According to that the we will install a package for the needed arch when cross building.

- Add a test for this case.
@LielHarel LielHarel force-pushed the feature/add-options-when-install-pkgs branch from a2b5bd2 to a22374e Compare September 18, 2023 18:50
@LielHarel
Copy link
Contributor Author

LielHarel commented Sep 18, 2023 •
edited
Loading

Thanks! I've rebased this against the release/2.0 branch - I'll fix the conflicts tomorrow :)

I have tried to solve the conflicts of my PR - I hope it helps
Pay attention that there is a commit fromdevelop I think that causes problems and I do not know if it really should be here.

@LielHarel LielHarel closed this Sep 18, 2023
@LielHarel LielHarel reopened this Sep 18, 2023
@memsharded
Copy link
Member

There was something breaking due to some concurrent changes (scons), not fault of this PR. I have updated the branch, hopefully now it will pass

memsharded
memsharded approved these changes Sep 18, 2023
View reviewed changes
conans/test/functional/toolchains/scons/test_sconsdeps.py Outdated Show resolved Hide resolved
conan/tools/system/package_manager.py Outdated Show resolved Hide resolved
conans/test/integration/tools/system/package_manager_test.py Outdated Show resolved Hide resolved
AbrilRBS
AbrilRBS approved these changes Sep 25, 2023
View reviewed changes
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.

Thanks a lot! This will be available in Conan 2.0.12 :)

@AbrilRBS AbrilRBS merged commit 5c91983 into conan-io:release/2.0 Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@memsharded memsharded memsharded approved these changes

@AbrilRBS AbrilRBS AbrilRBS approved these changes

Assignees

@AbrilRBS AbrilRBS

Labels
None yet
Projects
None yet
Milestone
2.0.12
Development

Successfully merging this pull request may close these issues.

[bug] Install packages compatible with incorrect architecture
5 participants
@LielHarel @CLAassistant @AbrilRBS @memsharded @czoido