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

Support nested Source roots #11

Closed
orsenthil opened this issue Apr 2, 2014 · 9 comments
Closed

Support nested Source roots #11

orsenthil opened this issue Apr 2, 2014 · 9 comments
Assignees

Comments

@orsenthil
Copy link
Contributor

Currently pants fails with mysterious error when it encounters targets with overlapping source root. It does not fail fast.

But discussions has led to the idea of supporting overlapping source root. The concept of SourceRoot seems applicable to IDEs and build system should be oblivious to it. Support of overlapping sourceroot would help. Even failing fast with correct error message would be helpful to the user before the support of overlapping source root lands.

@ericzundel
Copy link
Member

I want to provide a few details. The issue that I face that led to this report is a maven project with a second maven project nested beneath it.

/foo/pom.XML
/foo/BUILD
/foo/src/main/java
/foo/src/main/proto/
/foo/src/test/java

/foo/bar/pom.XML
/foo/bar/BUILD
/foo/bar/src/main/java
/foo/bar/src/main/proto
/foo/bar/src/test/java

The source roots are not really nested, but the project BUILD files where I defined java_library and java_protobuf_library are nested. The logic in sources.py assumes that given a target, you can find a single source root from it (before analyzing the source file list.) I took a quick stab at trying to infer the correct source root from the list of source files but hit a chicken and egg issue in some constructor calling SourceRoot.find()

@benjyw
Copy link
Contributor

benjyw commented Apr 3, 2014

I think that patrick's currently in-flight refactor of BUILD file parsing
and the target hierarchy also re-does source roots. For example, it gets
rid of the shady logic we currently have where if it can't find an
appropriate source root when it parses a target it lazily evaluates BUILD
files by climbing up the directory ancestry until it finds a source_root()
that works.

After his refactor the source_roots won't be lazily discovered, they'll
have to be declared up front in BUILD.bootstrap or wherever.

So I expect that he'll be able to support this kind of thing pretty easily,
basically your source root will be whichever one is "closest" to you in the
ancestry. I think this is what you'd expect, and also supports your use
case?

On Thu, Apr 3, 2014 at 6:01 AM, Eric Ayers [email protected] wrote:

I want to provide a few details. The issue that I face that led to this
report is a maven project with a second maven project nested beneath it.

/foo/pom.XML
/foo/BUILD
/foo/src/main/java
/foo/src/main/proto/
/foo/src/test/java

/foo/bar/pom.XML
/foo/bar/BUILD
/foo/bar/src/main/java
/foo/bar/src/main/proto
/foo/bar/src/test/java

The source roots are not really nested, but the project BUILD files where
I defined java_library and java_protobuf_library are nested. The logic in
sources.py assumes that given a target, you can find a single source root
from it (before analyzing the source file list.) I took a quick stab at
trying to infer the correct source root from the list of source files but
hit a chicken and egg issue in some constructor calling SourceRoot.find()


Reply to this email directly or view it on GitHubhttps://github.com//issues/11#issuecomment-39447696
.

@ericzundel
Copy link
Member

I'll have to look at the code. The problem as I saw it was not from the lazy evaluation when the BUILD file is underneath two prospective source roots.

source_root('/foo/src/main/proto/src', java_protobuf_library)
source_root('/foo/bar/src/main/proto/src', java_protobuf_library)

If you had a target foo/BUILD that contained a java_protobuf_library(name="proto", ...), how would it choose between foo/src/main/proto/src and foo/bar/src/main/proto/src as the "source root" for the target?

This is currently important for figuring out the appropriate --proto-path arguments to protoc.

@benjyw
Copy link
Contributor

benjyw commented Apr 3, 2014

Where are the sources of that java_protobuf_library?

On Thu, Apr 3, 2014 at 11:54 AM, Eric Ayers [email protected]:

I'll have to look at the code. The problem as I saw it was not from the
lazy evaluation when the BUILD file is underneath two prospective source
roots.

source_root('/foo/src/main/proto/src', java_protobuf_library)
source_root('/foo/bar/src/main/proto/src', java_protobuf_library)

If you had a target foo/BUILD that contained a
java_protobuf_library(name="proto", ...), how would it choose between
foo/src/main/proto/src and foo/bar/src/main/proto/src as the "source root"
for the target?

This is currently important for figuring out the appropriate --proto-path
arguments to protoc.


Reply to this email directly or view it on GitHubhttps://github.com//issues/11#issuecomment-39490411
.

@ericzundel
Copy link
Member

The sources would be where ever you defined them to be in sources[], but in maven, they would be under src/main/proto beneath the pom.xml file:

foo/src/main/proto/com/squareup/foo/foo.proto contains

import "squareup/com/foo/bar/bar.proto"
...

foo/pom.xml # contains maven deps for the foo/ project
foo/BUILD:

java_protobuf_library(
name = 'proto',
sources = [
'./src/main/proto/com/squareup/foo/foo.proto',
],
dependencies = [
'foo/bar:proto',
],
)

/foo/bar/src/main/proto/com/squareup/foo/bar/bar.proto

foo/bar/pom.xml # contains maven deps for the foo/bar project
foo/bar/BUILD:

java_protobuf_library(
name = 'proto',
sources = [
'./src/main/proto/com/squareup/foo/bar/bar.proto',
],
)

@benjyw
Copy link
Contributor

benjyw commented Apr 3, 2014

What I mean is, the answer to your question "how would it choose between
foo/src/main/proto/src and foo/bar/src/main/proto/src" should be that it'll
take whichever one is the closest ancestor to all of the sources.

Technically source_root should be a property of the payload, not of the
target.

On Thu, Apr 3, 2014 at 11:54 AM, Eric Ayers [email protected]:

I'll have to look at the code. The problem as I saw it was not from the
lazy evaluation when the BUILD file is underneath two prospective source
roots.

source_root('/foo/src/main/proto/src', java_protobuf_library)
source_root('/foo/bar/src/main/proto/src', java_protobuf_library)

If you had a target foo/BUILD that contained a
java_protobuf_library(name="proto", ...), how would it choose between
foo/src/main/proto/src and foo/bar/src/main/proto/src as the "source root"
for the target?

This is currently important for figuring out the appropriate --proto-path
arguments to protoc.


Reply to this email directly or view it on GitHubhttps://github.com//issues/11#issuecomment-39490411
.

wickman added a commit that referenced this issue Dec 4, 2014
SSIA.  The user facing parts of this change:
  - package resolution happens via requests now -- this is way more reliable
    and results in fewer Untranslateable exceptions
  - PEX_VERBOSE now controls all pex verbosity, including better messages around Untranslateable (yay!)
  - fixes a major pex regression with namespace packages introduced somewhere in 0.5.x

The pex 0.7.0 -> 0.8.0 changelog:

* *API change*: Decouple translation from package iteration.  This removes
  the Obtainer construct entirely, which likely means if you're using PEX as
  a library, you will need to change your code if you were doing anything
  nontrivial.  This adds a couple new options to ``resolve`` but simplifies
  the story around how to cache packages.
  [RB #785](https://rbcommons.com/s/twitter/r/785/)
* Refactor http handling in pex to allow for alternate http implementations.  Adds support
  for [requests](https://github.com/kennethreitz/requests),
  improving both performance and security.   For more information, read the commit notes at
  [91c7f32](pex-tool/pex@91c7f32).
  [RB #778](https://rbcommons.com/s/twitter/r/778/)
* Improvements to API documentation throughout.
* Renamed ``Tracer`` to ``TraceLogger`` to prevent nondeterministic isort ordering.
* Refactor tox.ini to increase the number of environment combinations and improve coverage.
* Adds HTTP retry support for the RequestsContext.
  [RB #1303](https://rbcommons.com/s/twitter/r/1303/)
* Make pex --version correct.
  [Issue #19](pex-tool/pex#19)
* Bug fix: Fix over-aggressive sys.modules scrubbing for namespace packages.  Under
  certain circumstances, namespace packages in site-packages could conflict with packages
  within a PEX, causing them to fail importing.
  [RB #1378](https://rbcommons.com/s/twitter/r/1378/)
* Bug fix: Replace uses of ``os.unsetenv(...)`` with ``del os.environ[...]``
  [Pull Request #11](pex-tool/pex#11)
* Bug fix: Scrub sys.path and sys.modules based upon both supplied path and
  realpath of files and directories.  Newer versions of virtualenv on Linux symlink site-packages
  which caused those packages to not be removed from sys.path correctly.
  [Issue #21](pex-tool/pex#21)
* Bug fix: The pex -s option was not correctly pulling in transitive dependencies.
  [Issue #22](pex-tool/pex#22)
* Bug fix: Adds ``content`` method to HTTP contexts that does HTML content decoding, fixing
  an encoding issue only experienced when using Python 3.
  [Issue #10](pex-tool/pex#10)

Testing Done:
build-support/bin/ci.sh

Reviewed at https://rbcommons.com/s/twitter/r/1421/
@jsirois
Copy link
Contributor

jsirois commented Nov 2, 2015

Assigning to @benjyw to close, but I think this can now be closed. Source roots are discovered unambiguously in overlapping cases now IIUC.

@benjyw
Copy link
Contributor

benjyw commented Nov 2, 2015

Yes, source roots are discovered unambiguously, and the way they do so is configured via options. Bootstrap BUILD files are gone, as is the problematic logic in SourceRoots.find().

@benjyw benjyw closed this as completed Nov 2, 2015
@gmalmquist
Copy link
Contributor

Hooray!

kwlzn added a commit that referenced this issue Mar 31, 2017
…n test. (#4407)

### Problem

Currently, on Linux the first thin client call to the daemon can deadlock just after the pantsd->fork->pantsd-runner workflow. Connecting to the process with `gdb` reveals a deadlock in the following stack in the `post_fork` `drop` of the `CpuPool`:

```
#0  0x00007f63f04c31bd in __lll_lock_wait () from /lib64/libpthread.so.0
No symbol table info available.
#1  0x00007f63f04c0ded in pthread_cond_signal@@GLIBC_2.3.2 () from /lib64/libpthread.so.0
No symbol table info available.
#2  0x00007f63d3cfa438 in notify_one ()
    at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/sys/unix/condvar.rs:52
No locals.
#3  notify_one () at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/sys_common/condvar.rs:39
No locals.
#4  notify_one () at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/sync/condvar.rs:208
No locals.
#5  std::thread::{{impl}}::unpark () at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/thread/mod.rs:633
No locals.
#6  0x00007f63d3c583d1 in crossbeam::sync::ms_queue::{{impl}}::push<futures_cpupool::Message> (self=<optimized out>, t=...)
    at /home/kwilson/.cache/pants/rust-toolchain/registry/src/github.com-1ecc6299db9ec823/crossbeam-0.2.10/src/sync/ms_queue.rs:178
        guard = <optimized out>
        self = <optimized out>
#7  0x00007f63d3c588ed in futures_cpupool::{{impl}}::drop (self=<optimized out>)
    at /home/kwilson/.cache/pants/rust-toolchain/git/checkouts/futures-rs-a4f11d094efefb0a/f7e6bc8/futures-cpupool/src/lib.rs:236
        self = 0x37547a0
#8  0x00007f63d3be871c in engine::fs::{{impl}}::post_fork (self=0x3754778) at /home/kwilson/dev/pants/src/rust/engine/src/fs.rs:355
        self = 0x3754778
#9  0x00007f63d3be10e4 in engine::context::{{impl}}::post_fork (self=0x37545b0)
    at /home/kwilson/dev/pants/src/rust/engine/src/context.rs:93
        self = 0x37545b0
#10 0x00007f63d3c0de5a in {{closure}} (scheduler=<optimized out>) at /home/kwilson/dev/pants/src/rust/engine/src/lib.rs:275
        scheduler = 0x3740580
#11 with_scheduler<closure,()> (scheduler_ptr=<optimized out>, f=...) at /home/kwilson/dev/pants/src/rust/engine/src/lib.rs:584
        scheduler = 0x3740580
        scheduler_ptr = 0x3740580
#12 engine::scheduler_post_fork (scheduler_ptr=0x3740580) at /home/kwilson/dev/pants/src/rust/engine/src/lib.rs:274
        scheduler_ptr = 0x3740580
#13 0x00007f63d3c1be8c in _cffi_f_scheduler_post_fork (self=<optimized out>, arg0=0x35798f0) at src/cffi/native_engine.c:2234
        _save = 0x34a65a0
        x0 = 0x3740580
        datasize = <optimized out>
#14 0x00007f63f07b5a62 in PyEval_EvalFrameEx () from /lib64/libpython2.7.so.1.0
```

This presents as a hang in the thin client, because the pailgun socket is left open in the pantsd-runner.

### Solution

Add pre-fork hooks and tear down the `CpuPool` instances prior to forking and rebuilding them.

### Result

Can no longer reproduce the hang.
lenucksi pushed a commit to lenucksi/pants that referenced this issue Apr 25, 2017
…n test. (pantsbuild#4407)

### Problem

Currently, on Linux the first thin client call to the daemon can deadlock just after the pantsd->fork->pantsd-runner workflow. Connecting to the process with `gdb` reveals a deadlock in the following stack in the `post_fork` `drop` of the `CpuPool`:

```
#0  0x00007f63f04c31bd in __lll_lock_wait () from /lib64/libpthread.so.0
No symbol table info available.
pantsbuild#1  0x00007f63f04c0ded in pthread_cond_signal@@GLIBC_2.3.2 () from /lib64/libpthread.so.0
No symbol table info available.
pantsbuild#2  0x00007f63d3cfa438 in notify_one ()
    at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/sys/unix/condvar.rs:52
No locals.
pantsbuild#3  notify_one () at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/sys_common/condvar.rs:39
No locals.
pantsbuild#4  notify_one () at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/sync/condvar.rs:208
No locals.
pantsbuild#5  std::thread::{{impl}}::unpark () at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/thread/mod.rs:633
No locals.
pantsbuild#6  0x00007f63d3c583d1 in crossbeam::sync::ms_queue::{{impl}}::push<futures_cpupool::Message> (self=<optimized out>, t=...)
    at /home/kwilson/.cache/pants/rust-toolchain/registry/src/github.com-1ecc6299db9ec823/crossbeam-0.2.10/src/sync/ms_queue.rs:178
        guard = <optimized out>
        self = <optimized out>
pantsbuild#7  0x00007f63d3c588ed in futures_cpupool::{{impl}}::drop (self=<optimized out>)
    at /home/kwilson/.cache/pants/rust-toolchain/git/checkouts/futures-rs-a4f11d094efefb0a/f7e6bc8/futures-cpupool/src/lib.rs:236
        self = 0x37547a0
pantsbuild#8  0x00007f63d3be871c in engine::fs::{{impl}}::post_fork (self=0x3754778) at /home/kwilson/dev/pants/src/rust/engine/src/fs.rs:355
        self = 0x3754778
pantsbuild#9  0x00007f63d3be10e4 in engine::context::{{impl}}::post_fork (self=0x37545b0)
    at /home/kwilson/dev/pants/src/rust/engine/src/context.rs:93
        self = 0x37545b0
pantsbuild#10 0x00007f63d3c0de5a in {{closure}} (scheduler=<optimized out>) at /home/kwilson/dev/pants/src/rust/engine/src/lib.rs:275
        scheduler = 0x3740580
pantsbuild#11 with_scheduler<closure,()> (scheduler_ptr=<optimized out>, f=...) at /home/kwilson/dev/pants/src/rust/engine/src/lib.rs:584
        scheduler = 0x3740580
        scheduler_ptr = 0x3740580
pantsbuild#12 engine::scheduler_post_fork (scheduler_ptr=0x3740580) at /home/kwilson/dev/pants/src/rust/engine/src/lib.rs:274
        scheduler_ptr = 0x3740580
pantsbuild#13 0x00007f63d3c1be8c in _cffi_f_scheduler_post_fork (self=<optimized out>, arg0=0x35798f0) at src/cffi/native_engine.c:2234
        _save = 0x34a65a0
        x0 = 0x3740580
        datasize = <optimized out>
pantsbuild#14 0x00007f63f07b5a62 in PyEval_EvalFrameEx () from /lib64/libpython2.7.so.1.0
```

This presents as a hang in the thin client, because the pailgun socket is left open in the pantsd-runner.

### Solution

Add pre-fork hooks and tear down the `CpuPool` instances prior to forking and rebuilding them.

### Result

Can no longer reproduce the hang.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants