Relative profile path should be valid for creation commands by uilianries · Pull Request #8685 · conan-io/conan

uilianries · 2021-03-22T21:25:33Z

Conan accepts absolute and relative paths for profiles, that's well documented.

We know the main recommendation is absolute, but relative path is not working well when is associated to same tree level, and profile names is same from default profile folder. If a default profile doesn't exist, it works well, but when duplicated, Conan prefers default profiles folder instead.

fixes #8678

Changelog: Fix: Accept relative profile path when folder is on same tree level.
Docs: conan-io/docs#2049

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.

^{Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.}

Signed-off-by: Uilian Ries <uilianries@gmail.com>

memsharded

It is great the modernizing of tests to pytest, thanks for taking the time!,

But on the other hand it is practically impossible to see if something might be breaking. Could you please split this into 2 PRs, one pure-refactor of tests, nothing else changing, and then the new fix with the new tests covering it?

Thanks!

uilianries · 2021-03-29T20:07:30Z

Yes, totally. Indeed is terrify for review.

Signed-off-by: Uilian Ries <uilianries@gmail.com>

uilianries · 2021-03-29T20:15:36Z

@memsharded Done

uilianries · 2021-03-29T20:19:44Z

Second PR: #8729

memsharded

This is quite risky:

I have reverted the changes to profile_loader.py, and the new tests are green! This means the tests are not testing this change, but just the previous existing functionality.
It is likely that we are breaking something, the condition might change for paths like relative/path/to/profile.txt that do not start with .
It is clear that the different paths functionality is not sufficiently covered by tests, we would need more thorough testing to make sure we are not breaking.

So I think we cannot merge these changes as-is.

Signed-off-by: Uilian Ries <uilianries@gmail.com>

uilianries · 2021-04-01T17:20:37Z

I have reverted the changes to profile_loader.py, and the new tests are green! This means the tests are not testing this change, but just the previous existing functionality.

Indeed is tricky, I needed to add profiles folder to reproduce same behavior. The commit 6b8de0d35d2 should reproduce that error.

It is likely that we are breaking something, the condition might change for paths like relative/path/to/profile.txt that do not start with .

The fragility is because we need to follow not equal profile names, but also profiles folder.

uilianries · 2021-04-01T17:56:58Z

Okay, unit test working as spectated: https://ci.conan.io/blue/organizations/jenkins/ConanTestSuite/detail/PR-8685/8/pipeline/#step-278-log-1537

Signed-off-by: Uilian Ries <uilianries@gmail.com>

memsharded · 2021-04-05T19:13:26Z

                             os=Windows"""), self.client.out)
+
+
+def test_profile_from_cache_path():


@uilianries please check this new test that I have provided. This would be proving that the previous fix was breaking, very likely massively breaking many users (all of them that manages profiles in subfolders in the cache).

The new proposed fix is more conservative and passes your new tests, please check if there is some other case to take into account.

I would love adding a functional test to validate relative paths too. I know those unit tests cover very well get_profile_path(), but I would like having and end-to-end, because we have a considerable stack before calling it.

For regression it can help us. WDYT?

Actually this should go to integration, not functional (as they don't depend on external tools), but until this is fixed, yes, it can co here.

Yes, it makes sense to have a few more tests here to cover better this functionality, please add some. Try to keep them simple, as the proposed one. Thanks!

Signed-off-by: Uilian Ries <uilianries@gmail.com>

memsharded

Please try the proposed simplification to tests.

Signed-off-by: Uilian Ries <uilianries@gmail.com>

memsharded · 2021-04-06T20:37:30Z

I have fixed the broken test in 8295aef. It was incorrectly managing paths, and it was creating folders in the code folder repo, making it dirty (and with risk to commit them accidentally)

uilianries added 2 commits March 22, 2021 18:19

conan-io#8678 Validate relative profile usage

141f8ac

Signed-off-by: Uilian Ries <uilianries@gmail.com>

conan-io#8678 Support relative profile folder

608b5b8

Signed-off-by: Uilian Ries <uilianries@gmail.com>

uilianries marked this pull request as ready for review March 22, 2021 21:36

uilianries changed the title ~~[WIP] Relative profile path should be valid for creation commands~~ Relative profile path should be valid for creation commands Mar 22, 2021

uilianries mentioned this pull request Mar 22, 2021

Add relative path for profiles conan-io/docs#2049

Merged

SSE4 reviewed Mar 23, 2021

View reviewed changes

Comment thread conans/client/profile_loader.py Outdated

memsharded reviewed Mar 23, 2021

View reviewed changes

Comment thread conans/test/functional/configuration/profile_test.py Outdated

uilianries added 4 commits March 23, 2021 09:24

conan-io#8678 Improve relative path validation

852a3e7

Signed-off-by: Uilian Ries <uilianries@gmail.com>

conan-io#8678 Covert profile_loader_test to pytest

479ea86

Signed-off-by: Uilian Ries <uilianries@gmail.com>

conan-io#8678 Add unit tests for relative read_profile

6289e3a

Signed-off-by: Uilian Ries <uilianries@gmail.com>

conan-io#8678 Revert functional profile test changes

c210fe7

Signed-off-by: Uilian Ries <uilianries@gmail.com>

uilianries commented Mar 23, 2021

View reviewed changes

Comment thread conans/test/unittests/client/profile_loader/profile_loader_test.py

uilianries requested review from SSE4 and memsharded March 23, 2021 14:02

uilianries added 3 commits March 23, 2021 11:11

conan-io#8678 Add unit test description

2b58608

Signed-off-by: Uilian Ries <uilianries@gmail.com>

conan-io#8678 Fix profile not found message

978e056

Signed-off-by: Uilian Ries <uilianries@gmail.com>

conan-io#8678 Fix expected path for Windows

6d5d235

Signed-off-by: Uilian Ries <uilianries@gmail.com>

memsharded reviewed Mar 28, 2021

View reviewed changes

conan-io#8678 Revert tests and add keep new ones

9e71614

Signed-off-by: Uilian Ries <uilianries@gmail.com>

uilianries mentioned this pull request Mar 29, 2021

Improve profile_loader_test.py with pytest #8729

Merged

5 tasks

memsharded requested changes Mar 29, 2021

View reviewed changes

memsharded added this to the 1.36 milestone Mar 29, 2021

conan-io#8678 Validate bug

6b8de0d

Signed-off-by: Uilian Ries <uilianries@gmail.com>

uilianries added 2 commits April 1, 2021 14:58

conan-io#8678 Apply fix

61fc7ad

Signed-off-by: Uilian Ries <uilianries@gmail.com>

conan-io#8678 Enforce profile folder name

731d39d

Signed-off-by: Uilian Ries <uilianries@gmail.com>

memsharded added 2 commits April 5, 2021 20:35

Merge branch 'develop' into hotfix/relative-profile-path

3e6c0ef

test proving proposed fix is breaking, and fix

34e25d1

memsharded reviewed Apr 5, 2021

View reviewed changes

conan-io#8678 Add functional tests

87c4671

Signed-off-by: Uilian Ries <uilianries@gmail.com>

uilianries requested a review from memsharded April 6, 2021 17:33

memsharded reviewed Apr 6, 2021

View reviewed changes

Comment thread conans/test/functional/configuration/profile_test.py Outdated

conan-io#8678 Simplify funcional tests

e98f110

Signed-off-by: Uilian Ries <uilianries@gmail.com>

uilianries requested a review from memsharded April 6, 2021 18:48

memsharded approved these changes Apr 6, 2021

View reviewed changes

fix test

8295aef

memsharded merged commit 5f5761a into conan-io:develop Apr 7, 2021

memsharded mentioned this pull request Apr 23, 2021

[bug] When specifying an absolute profiles path using pr:h <profile> and the same profile exists in the cache, the one in the cache is chosen #8854

Closed

		os=Windows"""), self.client.out)


		def test_profile_from_cache_path():

Conversation

uilianries commented Mar 22, 2021 • edited by czoido Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

memsharded left a comment

Choose a reason for hiding this comment

Uh oh!

uilianries commented Mar 29, 2021

Uh oh!

uilianries commented Mar 29, 2021

Uh oh!

uilianries commented Mar 29, 2021

Uh oh!

memsharded left a comment

Choose a reason for hiding this comment

Uh oh!

uilianries commented Apr 1, 2021

Uh oh!

uilianries commented Apr 1, 2021

Uh oh!

memsharded Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

uilianries Apr 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

memsharded Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

memsharded left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

memsharded commented Apr 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

uilianries commented Mar 22, 2021 •

edited by czoido

Loading

uilianries Apr 5, 2021 •

edited

Loading