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

add notes for Windows #10

Merged
merged 11 commits into from
May 7, 2018
Merged

add notes for Windows #10

merged 11 commits into from
May 7, 2018

Conversation

dirk-thomas
Copy link
Member

Addresses the problem described in colcon/colcon-notification#4.

@dirk-thomas dirk-thomas added the review Waiting for review (Kanban column) label May 4, 2018
@dirk-thomas dirk-thomas self-assigned this May 4, 2018
@dirk-thomas dirk-thomas requested a review from sloretz May 4, 2018 22:30
@sloretz
Copy link
Contributor

sloretz commented May 4, 2018

@dirk-thomas Success!

Mind if I add some commits to this branch? I'd like to add notes about

  • installing some dependencies to get the first build to succeed
  • adding COLCON_IGNORE the argcomplete package since it fails to build on windows
  • using curl to download the repos file since wget isn't available

Here's what worked for me

python -m venv env
call env\Scripts\activate
type nul > env\COLCON_IGNORE
pip install -U EmPy setuptools PyYAML pywin32 coloredlogs

curl --output colcon.repos https://raw.githubusercontent.com/colcon/colcon.readthedocs.org/master/colcon.repos
mkdir src
vcs import src < colcon.repos
type nul > src\colcon-argcomplete\COLCON_IGNORE

python src\colcon-core\bin\colcon build --paths src/*

call "install\setup.bat"
python install\colcon-core\Scripts\colcon-script.py build

@dirk-thomas
Copy link
Member Author

dirk-thomas commented May 4, 2018

Mind if I add some commits to this branch?

  • adding COLCON_IGNORE the argcomplete package since it fails to build on windows
  • using curl to download the repos file since wget isn't available

Sounds good to me.

  • installing some dependencies to get the first build to succeed

I would keep that change separate from this. Also I don't think that any of the ones you mentioned are necessary. They are all mention in the setup files and are being installed already. Anyway since there are many others I would keep it separate to not "block" this one from getting merged soon. See #11 for a stub.

@@ -23,6 +23,10 @@ While not strictly necessary it is recommended to use a virtual environment for
$ python3 -m venv /tmp/colcon-venv
$ . /tmp/colcon-venv/bin/activate

.. note::

On Windows the executable is likely name ``python`` and the script is located in ``/tmp/colcon-venv/Scripts/activate``.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/tmp on Windows ?
Maybe we can make all folder creations relative this way it should work on all platforms

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The windows note no longer references /tmp in 571e3f3

@dirk-thomas
Copy link
Member Author

Since the new "Fetch the sources" instructions for Windows work on all platforms I am fine with keeping them in a single section.

@sloretz
Copy link
Contributor

sloretz commented May 7, 2018

@mikaelarguedas Common download instructions in f0ecc1c

@dirk-thomas
Copy link
Member Author

👍 LGTM.

I also removed the usage of tmp for the venv (ba8507d) and changed the wording from "skip" to "ignore" since they have slightly different meaning in the context of package selection (e576298).

Copy link
Contributor

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

We could specify to use call on Windows for sourcing the setup files but I don't think its required to get this merged

@dirk-thomas
Copy link
Member Author

We could specify to use call on Windows for sourcing the setup files but I don't think its required to get this merged

I added about the script is being named and called on Windows in a7c192b (I didn't use call for it) and removed it from the activation script in a3229f9 (assuming that it was not required, maybe @sloretz can confirm?).

@sloretz
Copy link
Contributor

sloretz commented May 7, 2018

@dirk-thomas I just checked and can confirm it works both with and without call.

Edit: to clarify I mean activating the virtual environment works both with and without call.

@dirk-thomas dirk-thomas merged commit 8aeb195 into master May 7, 2018
@dirk-thomas dirk-thomas deleted the bootstrap_windows branch May 7, 2018 22:44
@dirk-thomas dirk-thomas removed the review Waiting for review (Kanban column) label May 7, 2018
@dirk-thomas
Copy link
Member Author

I added another related note in 455b739.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants