Feature/deterministic short paths #4238

wagdav · 2019-01-07T13:28:46Z

Changelog: Feature: Generate deterministic short paths on Windows
Docs: Omit

This PR implements the generation of the deterministic paths on Windows. The feature was proposed and discussed in #3971.

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

SSE4 · 2019-01-07T19:28:56Z

conans/util/windows.py

+    assert len(full_hash) > max_length
+
+    for length in range(min_length, max_length):
+        redirect = os.path.join(base, full_hash[:length])


are we running into possible race condition here in case of two CI jobs are trying to create hash redirect?

CI jobs should use their own short-paths cache, as they should use their own regular cache.

Otherwise, the cache locks per package, are done per package, so for the cases where conan aims to provide some concurrency over the cache, I think it should be already covered.

makes sense, thanks

memsharded

The PR is clear and well implemented, well tested. Great job!

There are a couple of small details, the policy for failing should be discussed, lets ask for @lasote feedback.

Thanks for contributing this feature!

memsharded · 2019-01-07T22:38:39Z

conans/util/windows.py

@@ -69,7 +71,9 @@ def path_shortener(path, short_paths):
        # cmd can fail if trying to set ACL in non NTFS drives, ignoring it.
        pass

-    redirect = tempfile.mkdtemp(dir=short_home, prefix="")
+    redirect = hashed_redirect(short_home, path) or tempfile.mkdtemp(dir=short_home, prefix="")


I think this could hide some problems, like CI sharing a short-paths cache, and having multiple CI jobs start increasing the path lenghts, of the hashed version until failing (instead of having a different short-paths cache per CI job).

I think I'd prefer to raise if a hashed short path can't be found in the range.

The fallback to mkdtemp was proposed by @jgsogo in this comment #3971 (comment)

I'll let you decide what's the best choice here. I'll adapt the PR when we reach consensus.

I vote for raise + explanation in the exception for the poor user on how to recover, maybe just pointing to a section in the conan documentation ?

If something goes wrong and a short-path is not deleted after removal of the corresponding package we will have a collision and we will run out of attempts, then it will raise.... the only solution for the user will be to enter the cache manually and remove some folders. We don't want the user to enter the cache and modify it, it should be automatically handled by Conan.

Nevertheless, we can write a warning asking the user to open an issue here in order to investigate what is happening with the package whose short-path is not being removed.

I added a warning if the deterministic short path creation fails.

conans/test/unittests/util/hashed_path_test.py

jgsogo

LGTM. Thanks for the contribution!

lasote · 2019-01-08T15:34:37Z

conans/util/windows.py

@@ -69,7 +71,9 @@ def path_shortener(path, short_paths):
        # cmd can fail if trying to set ACL in non NTFS drives, ignoring it.
        pass

-    redirect = tempfile.mkdtemp(dir=short_home, prefix="")
+    redirect = hashed_redirect(short_home, path) or tempfile.mkdtemp(dir=short_home, prefix="")
+    save(os.path.join(redirect, CONAN_REAL_PATH), path)


I can't see when this file is used. Shouldn't this file be used by hased_redirect() to see if the shortened path correspond already to a reference or something similar?
Imagine that I remove by hand the ~/.conan/data but the shortened folders are kept. Next thing I try to create the short path, it could collide and create tmp_folder. If we want that, why is this file necessary? Otherwise, is it file used?

We voted for this file in a whiteboard meeting we had to think about the implementation of this feature (conclusions are summarized in this comment: #3971 (comment)). This file is not used by Conan, it will serve to track some issues if those happen (the one you mention, a removal of a package not deleting the short directory,...). Also some users may find it useful too (#4163).

Ok, then please add a comment saying that the file is to debugging purposes only and not used by conan directly.

I added a comment explaining that this is for debugging purposes.

Generate deterministic short paths. This makes it possible to re-use the compilation cache database after the re-installation of conan packages. Resolves #3971

Resolves #3971

* Add sha256 helper function * Implement hashed_redirect Generate deterministic short paths. This makes it possible to re-use the compilation cache database after the re-installation of conan packages. Resolves conan-io#3971 * Write the full path to the Conan cache in the shortened directory. Resolves conan-io#3971

@lasote

* add new arch [armv5el, armv5hf] - armv5el -> armv5 soft-float - armv5hf -> armv5 hard-float There are still many devices based on these architectures especially embedded devices like Danelec devices, Synology etc... * support for armv5el, armv5hf - made changes to b2 generator - add tests * Feature/deterministic short paths (#4238) * Add sha256 helper function * Implement hashed_redirect Generate deterministic short paths. This makes it possible to re-use the compilation cache database after the re-installation of conan packages. Resolves #3971 * Write the full path to the Conan cache in the shortened directory. Resolves #3971 * Adds new architectures: ppc32, armv8_32 and armv8.3 (#4195) * new archs in settings.yaml * Added new archs to migration * Update migration to original settings.yml * Updated b2 generator with new archs * pure review of b2 generator * Added new archs to get_gnu_triplet() * revert changes on b2 generator * review * Back to aaarch64 and link for ilp32 clarification * Conversion to apple arch * added armv8.3 to get_gnu_triplet() * feature/enabling environment variables unset action (#4224) * Enabling the possibility of unsetting variables by means of appending such variable with value equal to None. Tests covering tools.env.environment_append have been included * Fixing PEP8 line width requirement * Fixing PEP8 line width requirement * Fixing path separator used in the unit test considering its os-dependant value * Fixing how the env_vars dictionary is being modified. Instead of removing values on-the-fly, It is previourly checked which should be removed by iterating on the keys. * Fixing how the env_vars dictionary is being modified again. Iterating on keys didn't work enough and now it is decoupled the iteration by using the keys that should be removed and were previously stored * Fix. Setting all the variables to None (unsetting all of them) effectively removes them from environment. - Applying changes needed. - Adding regression test. - Adding additional test covering the case where no variables are asked to be removed (empty dict). - Improve tests naming. * Removed versions (#4267) * Fixed bug with cached default profile (#4256) * Fixed bug * Docstring * removed caching in client_cache of settings and profile * Feature: link editable packages (#4181) * first try * fix includes * minor changes * more import fixes * fix more imports * short path as decorator * move SimplePaths to conans/paths/simple_paths.py * move back constants to conans.paths * moving folders to conans.paths * remove diffs caused just by ordering of imports * remove diffs caused just by ordering of imports * no diffs * no diff * move is_case_insensitive_os to conans.paths * add rm_conandir to windows imports * move package_layouts to paths * fix imports * move LINKED_FOLDER_SENTINEL to conans.paths.__init__.py * move 'get_package_layout' to 'SimplePaths' * get rid of PackageBaseLayout, go for duck typing * add parser for the file that will contain paths to includes, res, bins, libs,... * rename PackageUserLayout > PackageEditableLayout * interface for installed_as_editable info * test with a header only editable package * working example for header only lib * add test for a library with settings and options (still header only) * fix py2 test (declare string in source as unicode) * change import to avoid conans.tools one, so we need to mock a different function * make it compatible for win/linux * create the editable file sentinel using --editable argument in install command * add needed functions to simple_paths * add helper functions to simple paths * add package_metadata to new layouts * add missing named var (something went wrong merging from develop) * remove lines, not anymore there * add package metadata to users one * handle error: package folders needs a ConanFileReference * reorder imports * add missing import * reorder imports * if an editable package is dirty, log an error * moved parser to conan_file * move parsing for conan_package_layout closer to the ConanFile, we will be able to change it in the future * placeholders should be preprended with settings|options * fix typos, remove unused code * fix bug/typo * order imports * remove unreachable code * remove unreachable code * remove comments * move unittests to its folder * install as editable (requires the package to be already in the cache) * declare tests * refactor the editable hack: rely on member attribute * move imports * rename file to match class name * notify user about editable condition * gather logic together * make it editable from the very beginning, we don't care if it is editable or not * any error will require us to delete the editable flag * add tests * add tests related to editable package removal * rename tests * add test for other commands * add tests for conan commands * Update conans/test/command/editable_reference/create_editable_package_test.py Co-Authored-By: jgsogo <jgsogo@gmail.com> * add changes from review * move tests to final location * remove unused var * change variable name (it was a py conanfile) * don0t care about the output, but check something * remove comments * add tests installing an editable package (and installing its dependencies) * remove unused import * move cpp_info overrides to an standalone model * minor changes related to reviews * mode create/remove editables to client_cache class * renamed test folder * remove code related to 'install --editable' * link/unlink editable packages * consider these tests * handle remove command on editable * adapt message * removal is on its own test * check after unlink * add tests using linked packages * add tests for several commands * fix test related to utf8 and ordering * apply ordering * remove prints * putting path between quotes seems to solve windows issue * review (add more checks in tests) * function changed its name * no import in __init__.py * remove installed as editable * order is not guaranteed on info command output * no need for os.path.join * move EDITABLE before locking package layout * no different behaviour for update * new model for editable_cpp_info * check that cache file has more priority than the repo one * review * remove test, it is accepted behaviour now * review * revert change, this line is needed to pass tests in Windows * weird substitution is not needed, but path normalization is * inform about link removal * use conditional 'strict' arg when reusing cache for several TestClients * pass string to conan_api, build ConanFileReference inside it * add tests case for editable package without files, it will use package_info data * remove unused import * remove uneeded command * get the path to the conanfile.py from the argument to 'link' command inside the 'conan_api' * remove 'EditableCppInfo.create', rename loader ones to 'load(file...)' and 'loads(teext...)' * rename func argument * move CONAN_PACKAGE_LAYOUT_FILE to conans/paths * reorder variables inside file * renamed variables * pep8 * not using revisions in editable nodes, fix ordering issue * rename variables * fixing version ranges with spaces (#4273) * fixing version ranges with spaces * fix broken test * simplified testing * more error cases * [WIP] Rename common variables to conventional ones (#4272) * rename internal member 'PackageReference.conan' to 'PackageReference.ref' * rename internal member 'PackageReference.conan' to 'PackageReference.ref' * working on ConanFileRefs * working on ConanFileRefs * working on PackageReference * working on ClientCache * back to ConanFileReference * rename member 'Node::conan_ref' * rename member 'PythonRequires::conan_ref' * renaming vars in comments, docs,... * lost references * remove duplicated uneeded conversion to str * some loose fringes * minor fix, review by @lasote #4272 * rename PackageReference::package_id' to 'PackageReference::id' * loose fringes * support for armv5el, armv5hf - made changes to b2 generator - add tests * add handling for arm5hf to get_gnu_triplet

memsharded added this to the 1.12 milestone Jan 7, 2019

SSE4 reviewed Jan 7, 2019

View reviewed changes

memsharded approved these changes Jan 7, 2019

View reviewed changes

memsharded added the contributor pr label Jan 7, 2019

memsharded assigned lasote Jan 7, 2019

memsharded mentioned this pull request Jan 8, 2019

Make short paths more readable #4163

Closed

3 tasks

jgsogo approved these changes Jan 8, 2019

View reviewed changes

lasote requested changes Jan 8, 2019

View reviewed changes

David Wagner added 3 commits January 9, 2019 15:48

Add sha256 helper function

55bb3d7

Implement hashed_redirect

349524c

Generate deterministic short paths. This makes it possible to re-use the compilation cache database after the re-installation of conan packages. Resolves #3971

Write the full path to the Conan cache in the shortened directory.

1f40274

Resolves #3971

lasote approved these changes Jan 10, 2019

View reviewed changes

lasote merged commit 152451f into conan-io:develop Jan 10, 2019

wagdav deleted the feature/deterministic-short-paths branch January 10, 2019 09:25

jgsogo mentioned this pull request Jan 30, 2019

Running into issue with short names #4079

Closed

jgsogo mentioned this pull request Jul 26, 2019

Add an attribute to set the CONAN_USER_HOME_SHORT on the newConanClient construction #5541

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/deterministic short paths #4238

Feature/deterministic short paths #4238

wagdav commented Jan 7, 2019 •

edited by memsharded

SSE4 Jan 7, 2019

memsharded Jan 7, 2019

SSE4 Jan 7, 2019

memsharded left a comment

memsharded Jan 7, 2019

wagdav Jan 8, 2019

marco-m Jan 8, 2019

jgsogo Jan 8, 2019

wagdav Jan 9, 2019

jgsogo left a comment

lasote Jan 8, 2019

jgsogo Jan 8, 2019

lasote Jan 9, 2019

wagdav Jan 9, 2019 •

edited

Feature/deterministic short paths #4238

Feature/deterministic short paths #4238

Conversation

wagdav commented Jan 7, 2019 • edited by memsharded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memsharded left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgsogo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wagdav Jan 9, 2019 • edited

Choose a reason for hiding this comment

wagdav commented Jan 7, 2019 •

edited by memsharded

wagdav Jan 9, 2019 •

edited