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.

Sign up for GitHub

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

Jump to bottom

Windows top level build instructions #126

Merged
merged 9 commits into from
Jan 21, 2021
Merged

Windows top level build instructions #126

merged 9 commits into from
Jan 21, 2021

Conversation

JShep1
Copy link

@JShep1 JShep1 commented Jan 6, 2021 •
edited
Loading

Signed-off-by: John Shepherd [email protected]

Fixes #96

add windows citadel build
ce5d3e8 
Signed-off-by: John Shepherd <[email protected]>
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jan 6, 2021
Remove windows notes
c98b7cc 
Signed-off-by: John Shepherd <[email protected]>
@chapulina chapulina added the documentation Improvements or additions to documentation label Jan 6, 2021
@chapulina chapulina mentioned this pull request Jan 7, 2021
Open
13 tasks
Minor fixes
2690dd9 
Signed-off-by: John Shepherd <[email protected]>
@JShep1 JShep1 marked this pull request as ready for review January 7, 2021 20:24
@JShep1
Copy link
Author

JShep1 commented Jan 7, 2021

Marking ready for review. I made this guide generalizable to any version of ign-gazebo so it's an easy copy paste. Even though ign-gazebo isn't yet supported, I chose it as the package to build up to in the source build, I thought it would be more straightforward than specifying all of the children dependencies of ign-gazebo

I was wondering if we now want to make an effort to make Windows versions of all the tutorials in all of the separate repos. Such as https://github.com/ignitionrobotics/ign-gui/blob/ign-gui4/tutorials/06_example.md

fixes
acde48b 
Signed-off-by: John Shepherd <[email protected]>
@JShep1 JShep1 linked an issue Jan 7, 2021 that may be closed by this pull request
Top-level Windows installation instructions #96
Closed
chapulina
chapulina reviewed Jan 14, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I haven't tried it on Windows, just have some high-level comments.

I was wondering if we now want to make an effort to make Windows versions of all the tutorials in all of the separate repos

That's what @mabelzhang has been doing on #117, right?

citadel/install.md Outdated Show resolved Hide resolved
citadel/install_windows.md Outdated Show resolved Hide resolved
@mabelzhang
Copy link
Contributor

mabelzhang commented Jan 14, 2021 •
edited
Loading

That's what @mabelzhang has been doing on #117, right?

I think he might be referring to the OS-specific commands in the feature tutorials, not the installation tutorials. Like, are users on their own to figure out what the equivalent commands are on Windows for the different features, or do we want to be responsible for testing all of them and making sure they work on Windows.

@chapulina
Copy link
Contributor

I think he might be referring to the OS-specific commands in the feature tutorials,

Ah yes, got it! This is out of the scope for now, but something that needs to be done in the future.

mabelzhang
mabelzhang reviewed Jan 15, 2021
View reviewed changes
Copy link
Contributor

@mabelzhang mabelzhang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I haven't tried the colcon commands yet. Just formatting comments.

Is there a way to preview these locally? I didn't find a CMake file. Is it Doxygen or something else?

citadel/install_windows_src.md Outdated Show resolved Hide resolved
citadel/install_windows_src.md Outdated Show resolved Hide resolved
mabelzhang
mabelzhang reviewed Jan 15, 2021
View reviewed changes
Copy link
Contributor

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

I ran the tutorial. Compiling will take a while. Just a few comments on details.

citadel/install_windows.md Outdated Show resolved Hide resolved
citadel/install_windows_src.md Outdated Show resolved Hide resolved
citadel/install_windows_src.md Outdated Show resolved Hide resolved
citadel/install_windows_src.md Outdated Show resolved Hide resolved
citadel/install_windows_src.md Outdated Show resolved Hide resolved
citadel/install_windows_src.md Show resolved Hide resolved
citadel/install_windows_src.md Outdated Show resolved Hide resolved
citadel/install_windows_src.md Outdated Show resolved Hide resolved
John Shepherd added 3 commits January 15, 2021 14:54
suggested fixes
72f9eba 
Signed-off-by: John Shepherd <[email protected]>
Merge branch 'jshep1/top_level_windows_install' of https://github.com…
ee864c5 
…/ignitionrobotics/docs into jshep1/top_level_windows_install

Signed-off-by: John Shepherd <[email protected]>
Spacing and phrasing
6e4b398 
Signed-off-by: John Shepherd <[email protected]>
mabelzhang
mabelzhang approved these changes Jan 16, 2021
View reviewed changes
Copy link
Contributor

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments! Colcon built for me as described. Looks good. Approving for content. Feel free to adjust formatting as needed.
@chapulina is there a way to preview these locally like we can with the Doxygen docs? The build_docs.sh seems to upload straight to the web.

@chapulina
Copy link
Contributor

is there a way to preview these locally like we can with the Doxygen docs?

No 😢 #85

@mabelzhang
Copy link
Contributor

Perfect description 😅
Alright then I have no more comments. We won't know if that Getting Started link is fixed till it's deployed.

chapulina
chapulina approved these changes Jan 20, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I just have some nitpicks. Let's merge and deploy to see what it looks like!

citadel/install.md Outdated Show resolved Hide resolved
citadel/install.md Outdated Show resolved Hide resolved
citadel/install_windows_src.md Outdated Show resolved Hide resolved
citadel/install_windows_src.md Show resolved Hide resolved
nits
3618896 
Signed-off-by: John Shepherd <[email protected]>
@JShep1 JShep1 merged commit 9f3ebb9 into master Jan 21, 2021
@JShep1 JShep1 deleted the jshep1/top_level_windows_install branch January 21, 2021 01:51
@chapulina chapulina mentioned this pull request Mar 10, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

@mabelzhang mabelzhang mabelzhang approved these changes

@maryaB-osr maryaB-osr Awaiting requested review from maryaB-osr

@j-rivero j-rivero Awaiting requested review from j-rivero

Assignees
No one assigned
Labels
🏰 citadel Ignition Citadel documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Top-level Windows installation instructions
3 participants
@JShep1 @mabelzhang @chapulina