Re: Review Request 40131: Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.

[Joseph Wu]

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

Ship it!


LGTM.

- Joseph Wu


On Nov. 16, 2015, 1:32 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40131/
> ---
> 
> (Updated Nov. 16, 2015, 1:32 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.
> 
> 
> Diffs
> -
> 
>   bootstrap.bat PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40131/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 40131: Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.

[Alex Clemmer]

> On Nov. 11, 2015, 9:47 p.m., Joseph Wu wrote:
> > bootstrap.bat, line 40
> > 
> >
> > `mklink` only works with the Windows command prompt, run as an 
> > administrator.  Is that correct?
> > 
> > (At least, I don't think the command exists in other Windows shells, 
> > like PowerShell.)
> 
> Alex Clemmer wrote:
> That's mostly correct, yes: we do not assume the user has PowerShell 
> right now, and we do assume the shell running this is `cmd.exe`.
> 
> The symlink thing is a bit more subtle, though: creating symlinks in 
> general requires the `SeCreateSymbolicLinkPrivilege` privilege, which is by 
> default only granted to administrators. This need not be the case, though.
> 
> Joseph Wu wrote:
> We definitely don't want to assume any specific shell without at least a 
> comment saying so.  If there's some way to detect the shell, that would be 
> better.  (And PowerShell is, by default, installed on Windows nowadays.)

Interesting, it sounds like oyu know some stuff about Windows scripting that I 
don't. My impression was that if you wanted to make a batch script run in 
PowerShell, you specifically had to make it (_e.g._) a `.ps1`, and that 
everything else is assumed to be a batch script meant to be run with `cmd`. Is 
this wrong?

And if it is wrong, how does this affect with our current use of `cmd`-only 
`.bat` scripts (from what I can tell anyway) such as: 
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/cmake/macros/VsBuildCommand.bat


- Alex


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


On Nov. 16, 2015, 8:30 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40131/
> ---
> 
> (Updated Nov. 16, 2015, 8:30 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.
> 
> 
> Diffs
> -
> 
>   bootstrap.bat PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40131/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 40131: Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.

[Alex Clemmer]

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

(Updated Nov. 16, 2015, 9:32 p.m.)


Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
Remoortere, and Joseph Wu.


Repository: mesos


Description
---

Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.


Diffs (updated)
-

  bootstrap.bat PRE-CREATION 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 40131: Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.

[Alex Clemmer]

> On Nov. 11, 2015, 9:47 p.m., Joseph Wu wrote:
> > bootstrap.bat, line 52
> > 
> >
> > The second argument to `mklink` is relative to the first argument, so 
> > the `\ ` will be incorrect.
> > 
> > Ditto for below.

Ugh, you're right, how embarrassing. I'm not sure how I missed this.


- Alex


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


On Nov. 16, 2015, 8:30 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40131/
> ---
> 
> (Updated Nov. 16, 2015, 8:30 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.
> 
> 
> Diffs
> -
> 
>   bootstrap.bat PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40131/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 40131: Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.

[Alex Clemmer]

> On Nov. 11, 2015, 9:47 p.m., Joseph Wu wrote:
> > bootstrap.bat, line 40
> > 
> >
> > `mklink` only works with the Windows command prompt, run as an 
> > administrator.  Is that correct?
> > 
> > (At least, I don't think the command exists in other Windows shells, 
> > like PowerShell.)
> 
> Alex Clemmer wrote:
> That's mostly correct, yes: we do not assume the user has PowerShell 
> right now, and we do assume the shell running this is `cmd.exe`.
> 
> The symlink thing is a bit more subtle, though: creating symlinks in 
> general requires the `SeCreateSymbolicLinkPrivilege` privilege, which is by 
> default only granted to administrators. This need not be the case, though.
> 
> Joseph Wu wrote:
> We definitely don't want to assume any specific shell without at least a 
> comment saying so.  If there's some way to detect the shell, that would be 
> better.  (And PowerShell is, by default, installed on Windows nowadays.)
> 
> Alex Clemmer wrote:
> Interesting, it sounds like oyu know some stuff about Windows scripting 
> that I don't. My impression was that if you wanted to make a batch script run 
> in PowerShell, you specifically had to make it (_e.g._) a `.ps1`, and that 
> everything else is assumed to be a batch script meant to be run with `cmd`. 
> Is this wrong?
> 
> And if it is wrong, how does this affect with our current use of 
> `cmd`-only `.bat` scripts (from what I can tell anyway) such as: 
> https://github.com/apache/mesos/blob/master/3rdparty/libprocess/cmake/macros/VsBuildCommand.bat
> 
> Joseph Wu wrote:
> I'm not sure if PowerShell is smart enough to run `.bat` scripts with 
> something equivalent to `cmd \c `.  
> 
> I don't have a Windows computer handy to confirm this at the moment.  But 
> it would be straightforward to do so:
> ```
> echo "mklink" > test.bat
> ```
> In PowerShell:
> ```
> ./test.bat
> ```
> Then check which error message you get.  Is it a command-not-found error? 
>  Or `mklink` s help message?

I've confirmed that PowerShell will gracefully dipatch to `cmd` for `.bat` 
scripts, including both your test case above, and the actual bootstrap file.

I think this resolves the issue? Let me know if you have oter issues here.


- Alex


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


On Nov. 16, 2015, 8:30 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40131/
> ---
> 
> (Updated Nov. 16, 2015, 8:30 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.
> 
> 
> Diffs
> -
> 
>   bootstrap.bat PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40131/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 40131: Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.

[Joseph Wu]

> On Nov. 11, 2015, 1:47 p.m., Joseph Wu wrote:
> > bootstrap.bat, line 40
> > 
> >
> > `mklink` only works with the Windows command prompt, run as an 
> > administrator.  Is that correct?
> > 
> > (At least, I don't think the command exists in other Windows shells, 
> > like PowerShell.)
> 
> Alex Clemmer wrote:
> That's mostly correct, yes: we do not assume the user has PowerShell 
> right now, and we do assume the shell running this is `cmd.exe`.
> 
> The symlink thing is a bit more subtle, though: creating symlinks in 
> general requires the `SeCreateSymbolicLinkPrivilege` privilege, which is by 
> default only granted to administrators. This need not be the case, though.
> 
> Joseph Wu wrote:
> We definitely don't want to assume any specific shell without at least a 
> comment saying so.  If there's some way to detect the shell, that would be 
> better.  (And PowerShell is, by default, installed on Windows nowadays.)
> 
> Alex Clemmer wrote:
> Interesting, it sounds like oyu know some stuff about Windows scripting 
> that I don't. My impression was that if you wanted to make a batch script run 
> in PowerShell, you specifically had to make it (_e.g._) a `.ps1`, and that 
> everything else is assumed to be a batch script meant to be run with `cmd`. 
> Is this wrong?
> 
> And if it is wrong, how does this affect with our current use of 
> `cmd`-only `.bat` scripts (from what I can tell anyway) such as: 
> https://github.com/apache/mesos/blob/master/3rdparty/libprocess/cmake/macros/VsBuildCommand.bat

I'm not sure if PowerShell is smart enough to run `.bat` scripts with something 
equivalent to `cmd \c `.  

I don't have a Windows computer handy to confirm this at the moment.  But it 
would be straightforward to do so:
```
echo "mklink" > test.bat
```
In PowerShell:
```
./test.bat
```
Then check which error message you get.  Is it a command-not-found error?  Or 
`mklink` s help message?


- Joseph


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


On Nov. 16, 2015, 12:30 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40131/
> ---
> 
> (Updated Nov. 16, 2015, 12:30 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.
> 
> 
> Diffs
> -
> 
>   bootstrap.bat PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40131/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 40131: Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.

[Alex Clemmer]

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

(Updated Nov. 16, 2015, 8:30 a.m.)


Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
Remoortere, and Joseph Wu.


Repository: mesos


Description
---

Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.


Diffs (updated)
-

  bootstrap.bat PRE-CREATION 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 40131: Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.

[Alex Clemmer]

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

(Updated Nov. 16, 2015, 8:30 a.m.)


Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
Remoortere, and Joseph Wu.


Repository: mesos


Description
---

Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.


Diffs
-

  bootstrap.bat PRE-CREATION 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 40131: Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.

[Alex Clemmer]

> On Nov. 11, 2015, 9:47 p.m., Joseph Wu wrote:
> > bootstrap.bat, line 40
> > 
> >
> > `mklink` only works with the Windows command prompt, run as an 
> > administrator.  Is that correct?
> > 
> > (At least, I don't think the command exists in other Windows shells, 
> > like PowerShell.)

That's mostly correct, yes: we do not assume the user has PowerShell right now, 
and we do assume the shell running this is `cmd.exe`.

The symlink thing is a bit more subtle, though: creating symlinks in general 
requires the `SeCreateSymbolicLinkPrivilege` privilege, which is by default 
only granted to administrators. This need not be the case, though.


- Alex


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


On Nov. 16, 2015, 8:30 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40131/
> ---
> 
> (Updated Nov. 16, 2015, 8:30 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.
> 
> 
> Diffs
> -
> 
>   bootstrap.bat PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40131/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 40131: Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.

[Joseph Wu]

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



bootstrap.bat (line 40)


`mklink` only works with the Windows command prompt, run as an 
administrator.  Is that correct?

(At least, I don't think the command exists in other Windows shells, like 
PowerShell.)



bootstrap.bat (line 52)


The second argument to `mklink` is relative to the first argument, so the 
`\ ` will be incorrect.

Ditto for below.


- Joseph Wu


On Nov. 10, 2015, 3:10 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40131/
> ---
> 
> (Updated Nov. 10, 2015, 3:10 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.
> 
> 
> Diffs
> -
> 
>   bootstrap.bat PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40131/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 40131: Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [39537, 39538, 39539, 39540, 39541, 39383, 39559]

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

Error:
 2015-11-10 11:35:47 URL:https://reviews.apache.org/r/39559/diff/raw/ 
[10306/10306] -> "39559.patch" [1]
error: patch failed: 3rdparty/libprocess/3rdparty/stout/include/Makefile.am:65
error: 3rdparty/libprocess/3rdparty/stout/include/Makefile.am: patch does not 
apply
error: patch failed: 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp:83
error: 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp: patch does not 
apply
Failed to apply patch

- Mesos ReviewBot


On Nov. 10, 2015, 11:10 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40131/
> ---
> 
> (Updated Nov. 10, 2015, 11:10 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.
> 
> 
> Diffs
> -
> 
>   bootstrap.bat PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40131/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 40131: Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39537, 39538, 39539, 39540, 39541, 39383, 39559, 39219, 
39560, 39583, 39584, 39620, 39621, 39622, 39623, 39019, 39802, 39803, 39804, 
39805, 39834, 39850, 39851, 39852, 39888, 39889, 40102, 40131]

All tests passed.

- Mesos ReviewBot


On Nov. 10, 2015, 11:10 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40131/
> ---
> 
> (Updated Nov. 10, 2015, 11:10 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added `bootstrap.bat`, the Windows implementation of `bootstrap`.
> 
> 
> Diffs
> -
> 
>   bootstrap.bat PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40131/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>