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

Cleanup build instructions #523

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Wills2022
Copy link
Contributor

Description

Previous build instructions would not link dotnet library if the arcade machine was configured without a desktop environment. In a desktop-less environment ~/.bashrc is not loaded but ~/.bash.bash_profile is. This fixes this problem too.

The old .md file explicitly numbers points rather than using 1., 1.,1. to generate them automatically. This is a problem in this doc generally, but I will split that amendment into a separate pull if it's deemed an issue.

**update
The arcade cabinet build instructions direct users to a broken link on the Splashkit.io docs site.
The URL points to "https://splashkit.io/articles/installation/ubuntu/"

But this page as changed to https://splashkit.io/installation/linux/ at some point. It's possible to navigate to the correct page from the 404 in a couple of clicks, but there's no reason not to update this URL to point to the correct place.

Also replaced an ugly full link to sharepoint with an embedded link for better readability.

There's also a broken image in the "Configure Start CLI on Boot and Network Manager" section that pointed to a typo'd file that i have corrected.

The other links seem to be free from link rot.

Type of change

  • Documentation (update or new)

How Has This Been Tested?

.md file has been tested locally to ensure formatting is consistent and remains unbroken.

Testing Checklist:

  • Tested in latest Firefox

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have requested a review from ... on the Pull Request

Copy link

@Liquidscroll Liquidscroll left a comment

Choose a reason for hiding this comment

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

Typo on line 35 -- "latestes" (cant comment on this particular line)

Apart from duplicate export commands, there's a few typos to fix and make this perfect.


```shell
echo 'export DOTNET_ROOT=$HOME/.dotnet' >> ~/.bashrc
echo 'export PATH=$PATH:$HOME/.dotnet' >> ~/.bashrc
echo 'export DOTNET_ROOT=$HOME/.dotnet' >> ~/.bash.bash_profile
echo 'export PATH=$PATH:$HOME/.dotnet' >> ~/.bash_profile

Choose a reason for hiding this comment

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

We could source .bashrc from .bash_profile instead of duplicating export commands. If we do this, we must ensure that we don't make an infinite loop (.bash_profile sources .bashrc which sources .bash_profile...)

Not sure what's already in .bashrc - what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would that cause weird behavior if the user configured their Arcade Machine with a desktop environment? or are you suggesting that we setup bashrc to clone its config directly from .bash_profile?

(I'm not exceptionally familiar with .bash_profile as I typically setup everything under my local .bashrc)

Copy link

@Liquidscroll Liquidscroll Aug 6, 2024

Choose a reason for hiding this comment

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

I believe that you just put in source ~/.bashrc and it gets executed when .bash_profile loads so you can kind of chain them.

My understanding is that .bash_profile loads when you use a non-root shell.

EDIT: I'll approve this without changing this and get the input of the next reviewer, but I'm thinking it might be best to keep the changes as they are now.

Choose a reason for hiding this comment

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

Yeah it's a bit of a mess because history. Here's an excerpt from the bash man page

       ~/.bash_profile
              The personal initialization file, executed for login shells
       ~/.bashrc
              The individual per-interactive-shell startup file

So bash_profile is executed once when the user logs in (the definition of log in here is pretty vague so it ends up getting ran pretty frequently). bashrc is executed every time a new interactive session starts.

My recommendation would be to put the environment variable setting in bashrc. It shouldn't cause any weirdness with the desktop environment.

source ~/.bashrc
```

3. Verify the insalation with this command
1. Verify the insalation with this command

Choose a reason for hiding this comment

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

typo -- insalation -> installation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot. Embarrassing that survived a half dozen proof reads.

@@ -295,7 +302,7 @@ and one to wpa_supplicant:

4. Select Console Autologin

![Rasberry Pi Boot Option Screen](/docs/Splashkit/Applications/Arcade%20Machines/Arcade%20Machine%20Setup/Images/ConsoleAutoLogin.png)
![Rasberry Pi system Option Screen](/docs/Splashkit/Applications/Arcade%20Machines/Arcade%20Machine%20Setup/Images/ConsoleAutologin.png)

Choose a reason for hiding this comment

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

I'd prefer system to capitalised and therefore consistent with the rest of the phrase/sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I ended up changing this back to "Rasberry Pi Boot Option Screen" as this is what it was embedded as in the previous iteration of the document. When I was playing around with the markdown file to get it working I copied out the link from the next item, so the title ended up getting duplicated.

image

Updated re feedback form peer review.
Copy link

@hugglesfox hugglesfox left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of comments.


```shell
echo 'export DOTNET_ROOT=$HOME/.dotnet' >> ~/.bashrc
echo 'export PATH=$PATH:$HOME/.dotnet' >> ~/.bashrc
echo 'export DOTNET_ROOT=$HOME/.dotnet' >> ~/.bash.bash_profile

Choose a reason for hiding this comment

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

Should this be

Suggested change
echo 'export DOTNET_ROOT=$HOME/.dotnet' >> ~/.bash.bash_profile
echo 'export DOTNET_ROOT=$HOME/.dotnet' >> ~/.bash_profile

Copy link
Contributor

Choose a reason for hiding this comment

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

@Wills2022 Wondering about this too, just want to double check.


```shell
echo 'export DOTNET_ROOT=$HOME/.dotnet' >> ~/.bashrc
echo 'export PATH=$PATH:$HOME/.dotnet' >> ~/.bashrc
echo 'export DOTNET_ROOT=$HOME/.dotnet' >> ~/.bash.bash_profile
echo 'export PATH=$PATH:$HOME/.dotnet' >> ~/.bash_profile

Choose a reason for hiding this comment

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

Yeah it's a bit of a mess because history. Here's an excerpt from the bash man page

       ~/.bash_profile
              The personal initialization file, executed for login shells
       ~/.bashrc
              The individual per-interactive-shell startup file

So bash_profile is executed once when the user logs in (the definition of log in here is pretty vague so it ends up getting ran pretty frequently). bashrc is executed every time a new interactive session starts.

My recommendation would be to put the environment variable setting in bashrc. It shouldn't cause any weirdness with the desktop environment.

Copy link
Contributor

@WhyPenguins WhyPenguins left a comment

Choose a reason for hiding this comment

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

Hi Will! Again with this one, mind just fixing whatever issues the linter has? Sorry for having to do this so late, but really these shouldn't have passed peer review when the CI tests were failing.
Edit: Also let me know if it's literally just complaining about the numbering! If so maybe that's a setting we should change on the linter side 😃

@Wills2022
Copy link
Contributor Author

Hi Will! Again with this one, mind just fixing whatever issues the linter has? Sorry for having to do this so late, but really these shouldn't have passed peer review when the CI tests were failing. Edit: Also let me know if it's literally just complaining about the numbering! If so maybe that's a setting we should change on the linter side 😃

No worries, I've updated the file but the build still fails. According to the logs its getting stuck on these other documentation files:
image

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

Successfully merging this pull request may close these issues.

4 participants