-
-
Notifications
You must be signed in to change notification settings - Fork 628
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
WIP: Prototype an option for pants to create .pants.d as a symlink #8032
WIP: Prototype an option for pants to create .pants.d as a symlink #8032
Conversation
…sbuild#8024) It introduced failures in the nightly cron (pantsbuild#8023), and a bunch more errors when we tested it on Twitter internal targets.
…8006) * Introduce task_executor This is a single object we can pass around to allow all sorts of future running to happen, with logging happening properly, rather than needing to pass around different threadpools and manually sort out logging. * Core has an Executor not a Runtime * Scandir runs on io pool * PosixFS uses IO pool * ShardedLMDB uses io pool * Calculate fingeprint on io pool * Add TODO to move Executor some time * Add docstring to spawn_on_io_pool * Move Executor to its own crate * fmt
This means that we compile engine before we try running cbindgen over it, so we get useful parse errors out of rustc, rather than errors manifesting themselves as cbindgen going "There was a parse error somewhere!" I think ideally I would've moved the engine crate out into a new subcrate, but this way around better preserves git history and avoids merge conflicts.
### Problem As the purpose of the remoting is to make pants runs faster (for compile) it is very important to understand its performance. One of the nice ways is Zipkin tracing. ### Solution The result of the remote execution contains timings of different stages of the remote process that are used to create workunits to Zipkin trace. The last part of this PR is to add a test(unitest and/or integration test).
src/python/pants/util/dirutil.py
Outdated
@@ -66,7 +67,15 @@ def safe_mkdir(directory, clean=False): | |||
if clean: | |||
safe_rmtree(directory) | |||
try: | |||
os.makedirs(directory) | |||
if directory.endswith(get_repo_root() + '/.pants.d'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be better to use pathlib.PurePath(directory).name == ".pants.d" instead of checking if the path string ends with "/.pants.d", which will fail if the path ends with "/.pants.d/" for instance.
Note that os.basename doesn't work either because it'd return '' when called on "/foo/bar/baz/" (i.e. a path with a trailing /).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't use == ".pants.d"
because there are sub-dirs with the same name (".pants.d"). But added rstrip('/')
to ensure the directory path doesn't have /
suffix. E.g. : if directory.rstrip('/').endswith(get_workdir_symlink())
src/python/pants/util/dirutil.py
Outdated
os.makedirs(directory) | ||
if directory.endswith(get_repo_root() + '/.pants.d'): | ||
# make sure the base directory for .pants.d(symlink) exists | ||
if not os.path.isdir('/tmp/.pants.d'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.path.isdir may not be a good proxy for whether or not something exists at that path. .pants.d for instance could be a file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added both isfile and isdir check, thanks.
src/python/pants/util/dirutil.py
Outdated
|
||
# For .pants.d directory, since it is a symlink, use the base dir('/tmp/.pants.d') instead | ||
if src.endswith(get_repo_root() + '/.pants.d'): | ||
src = '/tmp/.pants.d' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be configurable and/or have a better default? /tmp is typically a tmpfs filesystem that doesn't persist across (among other things) a reboot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the plan is to add something like pants_physical_workdir: /path/to/physical_dir
in pants.ini
once this prototyping is complete. '/tmp/.pants.d'
is being used for the sake of simplicity.
src/python/pants/util/dirutil.py
Outdated
# For .pants.d directory, since it is a symlink, use the base dir('/tmp/.pants.d') instead | ||
if src.endswith(get_repo_root() + '/.pants.d'): | ||
src = '/tmp/.pants.d' | ||
if dst.endswith(get_repo_root() + '/.pants.d'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider using a better API. (See my comment above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(See my comment above)
Problem
When running
pants
on a virtual filesystem, any directory that is not under source control is implemented as a symbolic link to a directory on a physical ("real filesystem") volume. This would breakpants
because it currently requires.pants.d
directory (which is not under source control) to be a "real filesystem".Solution
.pants.d
directory as a symlink to a physical volume whenpants
runs and afterpants clean-all
.pants
's goals that fail due to.pants.d
's changes