Re: Review Request 47603: Agent:[2/2] Added Windows support for folder `launcher/`.

[Joris Van Remoortere]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47603/#review137686
---


Fix it, then Ship it!





src/launcher/executor.cpp (lines 78 - 82)


If you do this in `launcher/executor.hpp`, then why do it again here?



src/launcher/executor.cpp (line 160)


brace on new line



src/launcher/executor.cpp (line 162)


indentation



src/launcher/executor.cpp (line 166)


brace on new line



src/launcher/executor.cpp (lines 172 - 173)


space between functions



src/launcher/executor.cpp (line 402)


typo.



src/launcher/executor.cpp (line 405)


please use full names `processInformation`



src/launcher/executor.cpp (line 406)


indentation



src/launcher/executor.cpp (line 407)


new line.



src/launcher/posix/executor.hpp (lines 26 - 30)


You CAN NOT do this in header files.



src/launcher/posix/executor.hpp (lines 36 - 42)


I asked to have cpp files made for these.
What happened?



src/launcher/posix/executor.hpp (lines 37 - 38)


Where are the includes for these?



src/launcher/posix/executor.hpp (lines 49 - 54)


Please make changes like this separate patches next time.



src/launcher/posix/executor.hpp (line 52)


missing `:` after error description.



src/launcher/posix/executor.hpp (line 94)


Please be careful when rebasing. You copied your old code instead of 
adjusting to the new pattern for the conflict.
`NULL` -> `nullptr`



src/launcher/posix/executor.hpp (line 96)


nullptr



src/launcher/posix/executor.hpp (line 150)


Where are you sourcing `user` from now that you're no longer in the 
executor object's scope?
Did you try compiling this at all?



src/launcher/posix/executor.hpp (line 164)


indentation. double semicolon.



src/launcher/posix/executor.hpp (line 170)


Where are you sourcing `workingDirectory` from now that you're no longer in 
the executor object's scope?
Did you try compiling this at all?



src/launcher/posix/executor.hpp (line 174)


Where are you sourcing `sandboxDirectory` from now that you're no longer in 
the executor object's scope?
Did you try compiling this at all?



src/launcher/posix/executor.hpp (line 180)


indentation.



src/launcher/posix/executor.hpp (line 188)


indentation.



src/launcher/posix/executor.hpp (line 207)


Why are you changing this behavior?
This is very dangerous to do during a code move.
The original code uses `command` not `task.command`.



src/launcher/posix/executor.hpp (line 208)


nullptr



src/launcher/posix/executor.hpp (line 227)


indentation.



src/launcher/windows/executor.hpp (lines 38 - 43)


Same here. What happened to the cpp file we discussed?



src/launcher/windows/executor.hpp (line 53)


Same as above. Why are you using `task.command()` instead of `command`? The 
old code did not do this.



src/launcher/windows/executor.hpp (lines 70 - 71)


formatting



src/launcher/windows/executor.hpp (lines 72 - 73)


else goes on same line as closing brace



src/launcher/windows/executor.hpp (lines 78 - 79)


else goes on same line as closing brace



src/launcher/windows/executor.hpp (lines 89 - 96)


nullptr



src/launcher/windows/executor.hpp (line 90)

Re: Review Request 47603: Agent:[2/2] Added Windows support for folder `launcher/`.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47603/
---

(Updated June 8, 2016, 4:20 p.m.)


Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
Remoortere, and Michael Park.


Repository: mesos


Description
---

Agent:[2/2] Added Windows support for folder `launcher/`.


Diffs (updated)
-

  src/Makefile.am 29525c960e8fb2448260efdd774fd8fc1d68047b 
  src/exec/exec.cpp 15ebb7416701cea7f83adf178d1b56a5c6901102 
  src/launcher/executor.hpp PRE-CREATION 
  src/launcher/executor.cpp ffebb3a496be6a20396f8f539372a1e7527c9b6d 
  src/launcher/posix/executor.hpp PRE-CREATION 
  src/launcher/windows/executor.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/47603/diff/


Testing (updated)
---

OSX: make check


Thanks,

Alex Clemmer

Re: Review Request 47603: Agent:[2/2] Added Windows support for folder `launcher/`.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47603/
---

(Updated May 31, 2016, 3:31 a.m.)


Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
Remoortere, and Michael Park.


Repository: mesos


Description
---

Agent:[2/2] Added Windows support for folder `launcher/`.


Diffs (updated)
-

  src/Makefile.am f083cd762d5a070836cb3198897399479b8548c7 
  src/exec/exec.cpp 69a1fb256fe3005e814833ecac5a5a642b585510 
  src/launcher/executor.hpp PRE-CREATION 
  src/launcher/executor.cpp e4c3b75b647b54ffecedfdfa34fb3925f686c0c7 
  src/launcher/posix/executor.hpp PRE-CREATION 
  src/launcher/windows/executor.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/47603/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 47603: Agent:[2/2] Added Windows support for folder `launcher/`.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47603/
---

(Updated May 29, 2016, 2:15 a.m.)


Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
Remoortere, and Michael Park.


Repository: mesos


Description
---

Agent:[2/2] Added Windows support for folder `launcher/`.


Diffs (updated)
-

  src/Makefile.am f1bd5e6e5515535c2aaf9ea8b205f4e8c7096ec5 
  src/exec/exec.cpp 69a1fb256fe3005e814833ecac5a5a642b585510 
  src/launcher/executor.hpp PRE-CREATION 
  src/launcher/executor.cpp e4c3b75b647b54ffecedfdfa34fb3925f686c0c7 
  src/launcher/posix/executor.hpp PRE-CREATION 
  src/launcher/windows/executor.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/47603/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 47603: Agent:[2/2] Added Windows support for folder `launcher/`.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47603/
---

(Updated May 28, 2016, 3:17 a.m.)


Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
Remoortere, and Michael Park.


Changes
---

master rebase


Repository: mesos


Description
---

Agent:[2/2] Added Windows support for folder `launcher/`.


Diffs (updated)
-

  src/Makefile.am f1bd5e6e5515535c2aaf9ea8b205f4e8c7096ec5 
  src/exec/exec.cpp 69a1fb256fe3005e814833ecac5a5a642b585510 
  src/launcher/executor.hpp PRE-CREATION 
  src/launcher/executor.cpp e4c3b75b647b54ffecedfdfa34fb3925f686c0c7 
  src/launcher/posix/executor.hpp PRE-CREATION 
  src/launcher/windows/executor.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/47603/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 47603: Agent:[2/2] Added Windows support for folder `launcher/`.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47603/
---

(Updated May 28, 2016, 2:20 a.m.)


Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
Remoortere, and Michael Park.


Changes
---

master rebased


Repository: mesos


Description
---

Agent:[2/2] Added Windows support for folder `launcher/`.


Diffs (updated)
-

  src/Makefile.am f1bd5e6e5515535c2aaf9ea8b205f4e8c7096ec5 
  src/exec/exec.cpp 69a1fb256fe3005e814833ecac5a5a642b585510 
  src/launcher/executor.hpp PRE-CREATION 
  src/launcher/executor.cpp e4c3b75b647b54ffecedfdfa34fb3925f686c0c7 
  src/launcher/posix/executor.hpp PRE-CREATION 
  src/launcher/windows/executor.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/47603/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 47603: Agent:[2/2] Added Windows support for folder `launcher/`.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47603/#review135204
---



Bad patch!

Reviews applied: [47603, 47602, 47576, 47536, 47472, 47470, 47469, 47942, 
47412, 47411, 47410, 47409, 47404, 47403, 47391, 47390, 47389, 47388, 47387, 
47386, 47169, 47168, 41632, 47054, 47221, 47053, 47052]

Failed command: ./support/apply-review.sh -n -r 47576

Error:
2016-05-27 09:54:01 URL:https://reviews.apache.org/r/47576/diff/raw/ 
[14272/14272] -> "47576.patch" [1]
error: patch failed: src/slave/containerizer/mesos/containerizer.cpp:1182
error: src/slave/containerizer/mesos/containerizer.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/13366/console

- Mesos ReviewBot


On May 27, 2016, 6:57 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47603/
> ---
> 
> (Updated May 27, 2016, 6:57 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent:[2/2] Added Windows support for folder `launcher/`.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 710e1644e2f0a8e9b87cc997b2211291f4e055fd 
>   src/exec/exec.cpp a2e6d86fd0b1f3d688d17296151db74bcb9b3418 
>   src/launcher/executor.hpp PRE-CREATION 
>   src/launcher/executor.cpp fa4a89e97b0c427b4595a6adebc397aeb5bfaaa5 
>   src/launcher/posix/executor.hpp PRE-CREATION 
>   src/launcher/windows/executor.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47603/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47603: Agent:[2/2] Added Windows support for folder `launcher/`.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47603/
---

(Updated May 27, 2016, 6:57 a.m.)


Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
Remoortere, and Michael Park.


Repository: mesos


Description
---

Agent:[2/2] Added Windows support for folder `launcher/`.


Diffs (updated)
-

  src/Makefile.am 710e1644e2f0a8e9b87cc997b2211291f4e055fd 
  src/exec/exec.cpp a2e6d86fd0b1f3d688d17296151db74bcb9b3418 
  src/launcher/executor.hpp PRE-CREATION 
  src/launcher/executor.cpp fa4a89e97b0c427b4595a6adebc397aeb5bfaaa5 
  src/launcher/posix/executor.hpp PRE-CREATION 
  src/launcher/windows/executor.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/47603/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 47603: Agent:[2/2] Added Windows support for folder `launcher/`.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47603/#review135168
---



Bad patch!

Reviews applied: [47603, 47602, 47576, 47536, 47472, 47470, 47469, 47942, 
47412, 47411, 47410, 47409, 47404, 47403, 47391, 47390, 47389, 47388, 47387, 
47386, 47169, 47168, 41632, 47054, 47221, 47053, 47052]

Failed command: ./support/apply-review.sh -n -r 47576

Error:
2016-05-27 06:46:34 URL:https://reviews.apache.org/r/47576/diff/raw/ 
[14272/14272] -> "47576.patch" [1]
error: patch failed: src/slave/containerizer/mesos/containerizer.cpp:1182
error: src/slave/containerizer/mesos/containerizer.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/13362/console

- Mesos ReviewBot


On May 27, 2016, 5:38 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47603/
> ---
> 
> (Updated May 27, 2016, 5:38 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent:[2/2] Added Windows support for folder `launcher/`.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 710e1644e2f0a8e9b87cc997b2211291f4e055fd 
>   src/exec/exec.cpp a2e6d86fd0b1f3d688d17296151db74bcb9b3418 
>   src/launcher/executor.hpp PRE-CREATION 
>   src/launcher/executor.cpp fa4a89e97b0c427b4595a6adebc397aeb5bfaaa5 
>   src/launcher/posix/executor.hpp PRE-CREATION 
>   src/launcher/windows/executor.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47603/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47603: Agent:[2/2] Added Windows support for folder `launcher/`.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47603/
---

(Updated May 27, 2016, 5:38 a.m.)


Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
Remoortere, and Michael Park.


Repository: mesos


Description
---

Agent:[2/2] Added Windows support for folder `launcher/`.


Diffs (updated)
-

  src/Makefile.am 710e1644e2f0a8e9b87cc997b2211291f4e055fd 
  src/exec/exec.cpp a2e6d86fd0b1f3d688d17296151db74bcb9b3418 
  src/launcher/executor.hpp PRE-CREATION 
  src/launcher/executor.cpp fa4a89e97b0c427b4595a6adebc397aeb5bfaaa5 
  src/launcher/posix/executor.hpp PRE-CREATION 
  src/launcher/windows/executor.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/47603/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 47603: Agent:[2/2] Added Windows support for folder `launcher/`.

[Daniel Pravat]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47603/#review134053
---




src/launcher/windows/executor.hpp (line 107)


I've made a change to start the process in a job object. The patch is 
commited in your branch.


- Daniel Pravat


On May 19, 2016, 4:08 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47603/
> ---
> 
> (Updated May 19, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent:[2/2] Added Windows support for folder `launcher/`.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 447bc2ab511ad173d3d911be10992be0974f4584 
>   src/launcher/executor.hpp PRE-CREATION 
>   src/launcher/executor.cpp fa4a89e97b0c427b4595a6adebc397aeb5bfaaa5 
>   src/launcher/posix/executor.hpp PRE-CREATION 
>   src/launcher/windows/executor.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47603/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47603: Agent:[2/2] Added Windows support for folder `launcher/`.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47603/#review134039
---



Bad patch!

Reviews applied: [47603, 47602, 47576, 47536, 47472, 47471, 47470, 47469, 
47468, 47412, 47411, 47410, 47409, 47404, 47403, 47391, 47390, 47389, 47388, 
47387, 47386, 47169, 47168, 41632, 47054, 47221, 47053, 47052]

Failed command: ./support/apply-review.sh -n -r 47471

Error:
2016-05-19 21:25:51 URL:https://reviews.apache.org/r/47471/diff/raw/ 
[5804/5804] -> "47471.patch" [1]
error: patch failed: src/exec/exec.cpp:595
error: src/exec/exec.cpp: patch does not apply
error: patch failed: src/executor/executor.cpp:154
error: src/executor/executor.cpp: patch does not apply
error: patch failed: src/launcher/fetcher.cpp:443
error: src/launcher/fetcher.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/13186/console

- Mesos ReviewBot


On May 19, 2016, 4:08 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47603/
> ---
> 
> (Updated May 19, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent:[2/2] Added Windows support for folder `launcher/`.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 447bc2ab511ad173d3d911be10992be0974f4584 
>   src/launcher/executor.hpp PRE-CREATION 
>   src/launcher/executor.cpp fa4a89e97b0c427b4595a6adebc397aeb5bfaaa5 
>   src/launcher/posix/executor.hpp PRE-CREATION 
>   src/launcher/windows/executor.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47603/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 47603: Agent:[2/2] Added Windows support for folder `launcher/`.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47603/
---

Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
Remoortere, and Michael Park.


Repository: mesos


Description
---

Agent:[2/2] Added Windows support for folder `launcher/`.


Diffs
-

  src/Makefile.am 447bc2ab511ad173d3d911be10992be0974f4584 
  src/launcher/executor.hpp PRE-CREATION 
  src/launcher/executor.cpp fa4a89e97b0c427b4595a6adebc397aeb5bfaaa5 
  src/launcher/posix/executor.hpp PRE-CREATION 
  src/launcher/windows/executor.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/47603/diff/


Testing
---


Thanks,

Alex Clemmer