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

Option values read from @files don't invalidate when the file changes #10360

Open
benjyw opened this issue Jul 15, 2020 · 6 comments
Open

Option values read from @files don't invalidate when the file changes #10360

benjyw opened this issue Jul 15, 2020 · 6 comments
Labels

Comments

@benjyw
Copy link
Contributor

benjyw commented Jul 15, 2020

Any* option's value can be read from a file, by using @path/to/file as the value.

A subsystem instance that has such an option is not invalidated when the contents of the file change, so changing the file and then rerunning pants doesn't apply the new value, until you restart pantsd.

  • Unless it sets fromfile=False.
@benjyw benjyw added the bug label Jul 15, 2020
@benjyw benjyw added this to the 2.0.0.alpha0 milestone Jul 16, 2020
@stuhood stuhood changed the title Option values read from files don't invalidate when the file changes Option values read from @files don't invalidate when the file changes Jul 17, 2020
@stuhood
Copy link
Member

stuhood commented Jul 17, 2020

So, I think that a plan here is:

  • Remove the dependency from rust bootstrapping on BinaryUtil, which should allow options parsing to depend on the native code.
  • Introduce a "bootstrap" Scheduler instance which will be responsible for constructing the bootstrap options that are used to create the "real" Scheduler.
  • Refactor the relevant portion of options parsing (namely, anything that interacts with files) into @rules, initially breaking out of the sandbox with absolute file reads.
  • Add an absolute file read instrinsic, and use that to finally resolve this.

@Eric-Arellano
Copy link
Contributor

I like it. V2-ification of the options system.

However, this is a really big undertaking, so we should double check that this is a 2.0 priority. cc @benjyw. Is this only a broken window when pantsd is used?

@benjyw
Copy link
Contributor Author

benjyw commented Jul 17, 2020

It is, but it's a big one.

We could however just not support fromfile options in 2.0 and reintroduce them in 2.1?

@Eric-Arellano
Copy link
Contributor

We could however just not support fromfile options in 2.0 and reintroduce them in 2.1?

To confirm, something like --isort-config works okay, then, right? This is solely an issue with fromfile.

@stuhood
Copy link
Member

stuhood commented Jul 17, 2020

Yea... that and possibly the file_option and dir_option types breaking out of the sandbox?

It has a slight impact on performance, because we re-parse all config files for each run, and validate all option values rather than having them memoized... the engine would make that easy.

@stuhood
Copy link
Member

stuhood commented Jul 21, 2020

FWIW, the @file feature isn't even documented: https://www.pantsbuild.org/docs/options , so maybe we don't need to do anything here until closer to release.

@stuhood stuhood modified the milestones: 2.0.0.alpha0, 2.0.x Jul 21, 2020
@Eric-Arellano Eric-Arellano removed this from the 2.0.0 important milestone Oct 12, 2020
stuhood added a commit that referenced this issue Feb 22, 2021
### Problem

#11536 moves from using POSIX-level replacement of the `stdio` file descriptors `{0, 1, 2}`, to replacing `sys.std*` with a thread-local implementation. Unfortunately, Python `subprocess`/`Popen` APIs hardcode `{0, 1, 2}` rather than actually inspecting `sys.std*.fileno()`, and so usages of those APIs that use `std*=None` (i.e. "inherit" mode), will inherit intentionally dead/closed file handles under `pantsd`.

PEX uses inherit mode when spawning `pip` (in order to pass through `stderr` to the parent process), and this runs afoul of the above behavior. Pants has used PEX as a library for a very long time, but 95% of usecases have now migrated to using PEX as a binary, with wrappers exposed via the `@rule` API. The `PluginResolver` that Pants uses to resolve its own code is the last usage of PEX as a library. 

### Solution

In a series of commits, introduce a "bootstrap" `Scheduler` that is able to resolve plugins after an `OptionsBootstrapper` has been created, but before creating the `BuildConfiguration` (which contains plugin information).

### Result

Although Pants has some stray references to PEX APIs, it no longer uses PEX as a library to resolve dependencies. #11536 and #7654 are unblocked.

In future, if the options required to construct a minimal scheduler can be further pruned, the bootstrap `Scheduler` might also be able to be used to create the `OptionsBootstrapper`, which would allow for addressing #10360.
@stuhood stuhood removed their assignment Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants