First round of native engine feedback

Review Request #4359 — Created Nov. 7, 2016 and submitted

stuhood
pants
4037
pants-reviews
benjyw, jsirois, kwlzn, nhoward_tw

Applies the review feedback from r/4270, and fixes one minor issue with the bootstrap script.

  • exit 1 in build-support/bootstrap_native if compilation fails. This allows for iterating on e.g. ./pants --enable-v2-engine list .. until the native code successfully builds.
  • Used git-ls-files to determine which files to hash.
  • Derive/implement Default for Key/Value.
  • Whitespace in generic parameter lists.
  • Allow for visualizing executions to a directory via a --native-engine-visualize-to=$dir option.
  • Convert EntryId to a sealed type to avoid accidental random values.
  • Improve a few comments.
  • Suffix python keywords rather than prefixing them.
  • Whitespace fixes.
  • Silenced known unused code warning for 4020.
  • Attempt to cache ~/.cargo in travis.
  • Don't nuke $HOME in hermetic tests.

https://travis-ci.org/pantsbuild/pants/builds/173991370

JS
  1. It would be great to fix dylib -> lib or just use bin.

    It would also be awesome to fix this too:

    $ ./pants
       Compiling libc v0.2.15
       Compiling fnv v1.0.3
       Compiling engine v0.0.1 (file:///home/jsirois/dev/pantsbuild/pants/src/rust/engine)
    warning: method is never used: `merge`, #[warn(dead_code)] on by default
      --> src/rust/engine/src/core.rs:25:3
       |
    25 |   pub fn merge(&self, right: Variants) -> Variants {
       |   ^
    
  2. build-support/bootstrap_native (Diff revision 1)
     
     

    The other issue is the hash taken above is the wrong one, it should be taken again here after the cargo build to avoid the current sequence:

    1. hash miss -> cargo build
    2. hash miss (Cargo.lock?) -> cargo noop build
    3. hash hit
    1. I think this was likely because the target directory was being hashed. Have switched this to a git-ls-files for staged/cached files, which will ignore excluded files. Will still build twice immediately after a Cargo.toml edit, but othewise should be stable.

  3. 
      
ST
  1. Interesting. From the CI run:

    A rust installation could not be found, installing via the instructions at https://www.rustup.rs ...
    info: downloading installer
    info: updating existing rustup installation
    
    /home/travis/build/pantsbuild/pants/build-support/bootstrap_native: line 49: rustup: command not found
    /home/travis/build/pantsbuild/pants/build-support/bootstrap_native: line 62: cargo: command not found
    

    not finding the install, but then updating the install...

  2. 
      
ST
ST
JS
  1. 
      
  2. build-support/bootstrap_native (Diff revision 3)
     
     

    The || true was a remnant of me iterating with set -o errexit in effect. That had to be killed though due to the structure of the sourcing script and so this hack can be too.

  3. build-support/bootstrap_native (Diff revision 3)
     
     

    very minor, but build-support/common.sh is sourced so you have die available in place of exit 1.

  4. build-support/bootstrap_native (Diff revision 3)
     
     

    Seems to me its still worth re-hashing for the Cargo.lock case - its going to look buggy that there is 1 noop rebuild before true caching and we're a bunch of devs. I at least was bugged by the bug in my 1st RB.

  5. 
      
ST
ST
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as 09650a1e4e1f5ead484c4d16abc181cf8d9e88b3

Loading...