https://pantsbuild.org/ logo
Powered by
#development
  • r

    rough-minister-58256

    03/27/2017, 10:14 PM
    where’d I mention that?
  • r

    rough-minister-58256

    03/27/2017, 10:15 PM
    afaict, yes it is a thing we already need to do
  • w

    witty-crayon-22786

    03/27/2017, 10:15 PM
    Copy code
    but that actually creates the `File` object on your behalf (and requires external parent dir creation)
  • w

    witty-crayon-22786

    03/27/2017, 10:15 PM
    (your comment this morning)
  • r

    rough-minister-58256

    03/27/2017, 10:15 PM
    yeah, we need to do that either way
  • w

    witty-crayon-22786

    03/27/2017, 10:16 PM
    we currently create the tempdir once, and then create the dest dir for every snapshot
  • w

    witty-crayon-22786

    03/27/2017, 10:17 PM
    so yea, there'd be another syscall here to check that the parent directory exists before tempfileing. but i think it's cleaner than the alternative.
  • r

    rough-minister-58256

    03/27/2017, 10:20 PM
    well, yeah we’ll have to do that either way because of the situation presented with clean-all composite goal execution
  • w

    witty-crayon-22786

    03/27/2017, 10:20 PM
    @rough-minister-58256 : another thought would be to defer the tmpdir creation
  • r

    rough-minister-58256

    03/27/2017, 10:20 PM
    that happens inside 
    TempDir::new_in
    tho
  • w

    witty-crayon-22786

    03/27/2017, 10:20 PM
    ...oh. that doesn't work in the case of something like 
    compile clean-all test
    or something though =/
  • r

    rough-minister-58256

    03/27/2017, 10:20 PM
    yeah
  • r

    rough-minister-58256

    03/27/2017, 10:22 PM
    i wonder about the cost of something like 
    NamedTempFile
    doing random # gen for every temp file we create vs doing it once per tmpdir root
  • w

    witty-crayon-22786

    03/27/2017, 10:24 PM
    a syscall should dwarf random number generation
  • w

    witty-crayon-22786

    03/27/2017, 10:24 PM
    should/will
  • r

    rough-minister-58256

    03/27/2017, 10:24 PM
    its both, actually
  • w

    witty-crayon-22786

    03/27/2017, 10:24 PM
    in both cases there is a syscall
  • w

    witty-crayon-22786

    03/27/2017, 10:25 PM
    *shrug
  • r

    rough-minister-58256

    03/27/2017, 10:28 PM
    the odds of two concurrent 
    TempDir
    keying to the same randomized dir name across a 
    clean-all
    seems low to me - and we can minimize syscalls by just doing a singular 
    mkdir
    each time and checking the result of that - but I do agree the more correct approach is to recreate the 
    TempDir
    each time guarded with a 
    stat
    - then 
    mkdir
    - iff needed.
  • w

    witty-crayon-22786

    03/27/2017, 10:30 PM
    up to you.
  • w

    witty-crayon-22786

    03/27/2017, 10:46 PM
    nb: i deleted the 
    #native
    label in github, as it was mostly synonymous with 
    #engine
    at this point.
  • r

    rough-minister-58256

    03/27/2017, 10:47 PM
    word
  • w

    witty-crayon-22786

    03/28/2017, 12:40 AM
    going to make dinner a bit early. back later.
  • w

    witty-crayon-22786

    03/28/2017, 1:54 AM
    back.
  • w

    witty-crayon-22786

    03/28/2017, 3:12 AM
    if https://github.com/pantsbuild/pants/pull/4369 goes green tonight, will likely merge it to unblock other work
  • w

    witty-crayon-22786

    03/28/2017, 3:12 AM
    out for a bit
  • r

    rough-minister-58256

    03/28/2017, 3:30 AM
    still arguing w/ the compiler over the mutex fix on top of my PR
  • r

    rough-minister-58256

    03/28/2017, 4:22 AM
    Copy code
    error[E0495]: cannot infer an appropriate lifetime for autoref due to conflicting requirements
   --> src/rust/engine/src/fs.rs:816:26
    |
816 |     let temp_path = self.next_temp_path().expect("Couldnt get the next temp path.");
    |                          ^^^^^^^^^^^^^^
    |
note: first, the lifetime cannot outlive the anonymous lifetime #1 defined on the body at 813:90...
   --> src/rust/engine/src/fs.rs:813:91
    |
813 |   pub fn create(&self, fs: &PosixFS, paths: Vec<PathStat>) -> CpuFuture<Snapshot, String> {
    |                                                                                           ^
note: ...so that reference does not outlive borrowed content
   --> src/rust/engine/src/fs.rs:816:21
    |
816 |     let temp_path = self.next_temp_path().expect("Couldnt get the next temp path.");
    |                     ^^^^
    = note: but, the lifetime must be valid for the static lifetime...
note: ...so that the type `[closure@src/rust/engine/src/fs.rs:818:24: 835:6 temp_path:&std::path::Path, paths:std::vec::Vec<fs::PathStat>, build_root:fs::Dir, dest_dir:std::path::PathBuf]` will meet its required lifetime bounds
   --> src/rust/engine/src/fs.rs:818:15
    |
818 |     fs.pool().spawn_fn(move || {
    |               ^^^^^^^^
  • r

    rough-minister-58256

    03/28/2017, 4:26 AM
    got it
  • r

    rough-minister-58256

    03/28/2017, 5:38 AM
    https://github.com/pantsbuild/pants/pull/4373 should be reviewable, if you’d like to take one last peek at the mutex stuff.
1...299300301...906Latest