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

Fix and improve WSL process #1949

Merged
merged 15 commits into from
Feb 28, 2024
Merged

Fix and improve WSL process #1949

merged 15 commits into from
Feb 28, 2024

Conversation

MichaelBelgium
Copy link

@MichaelBelgium MichaelBelgium commented Feb 3, 2024

As every feature has this

if [ -f ~/.homestead-features/wsl_user_name ]; then
    WSL_USER_NAME="$(cat ~/.homestead-features/wsl_user_name)"
    WSL_USER_GROUP="$(cat ~/.homestead-features/wsl_user_group)"
else
    WSL_USER_NAME=vagrant
    WSL_USER_GROUP=vagrant
fi

The bin/wsl-init script does not create the homestead-features folder and neither the wsl_user_name/wsl_user_group file.

This PR "starts to fix" the command bin/homestead wsl:apply-features basicly, but I noticed there are more references to /home/vagrant in the features files. I believe they all have to be /home/$WSL_USER_NAME no?

eg

printf "\nPATH=\"/usr/local/go/bin:\$PATH\"\n" | tee -a /home/vagrant/.profile

This command would fail because /home/vagrant doesnt exist, except if the WSL user is called "vagrant". Would it be ok to replace all these references too?

EDIT: meanwhile changed all references.

This also covers the stuff i encountered here last year #1484 (comment)

The above, to provision and configure WSL with Ubuntu, is bit outdated now too.
It should be ubuntu 22.04 now, and the commands should be executed in this order, with sudo - after installing and logging in WSL/ubuntu 22.04:

sudo bin/wsl-init
sudo bin/homestead wsl:apply-features
sudo bin/homestead wsl:create-sites
sudo bin/homestead wsl:create-databases

Should be in the users home directory, not root
Just like other php versions
They do the same thing, prevent copy/pasting and what not
nginx didn't allow to restart as the restart command was in a loop and resulted in

```
Feb 03 15:09:01 systemd[1]: Failed to start A high performance web server and a reverse proxy server.
Feb 03 15:09:01 systemd[1]: nginx.service: Start request repeated too quickly.
Feb 03 15:09:01 systemd[1]: nginx.service: Failed with result 'exit-code'.
```

So just restart once at the end :)
@MichaelBelgium MichaelBelgium changed the title Fix WSL + apply-features process Fix and improve WSL process Feb 3, 2024
@karmendra
Copy link

What's the plan with this PR?

Actually I am working on more features for WSL provisioning, and all these changes seems to make sense. If there isn't any plan to merge this PR soon I will copy these changes to my branch manually.

Note that there are few conflicts to be fixed.

@MichaelBelgium
Copy link
Author

Without this PR homestead + WSL simply doesn't work properly, so it's kinda priority in my eyes and wanna see this merged in asap

Then you also can pull it into your fork and work on the provisioning more @karmendra

PS: solved the merge conflict

@karmendra
Copy link

@MichaelBelgium I've noticed another discrepancy in wsl-init: MySQL isn't being installed. The issue arises because there's currently no script to handle MySQL installation afterward. My understanding, based on the Vagrant installation and the settlers amd64.sh, is that Vagrant provisions with MySQL by default. If a user later updates the Homestead.yaml file to install MariaDB, it then uninstalls MySQL and installs MariaDB instead.

In light of this, I propose two potential solutions:

We maintain the current behavior in wsl-init by including MySQL installation.
We offer an option to install MySQL later by adding a script to remove MariaDB and install MySQL 8.

What are your thoughts?

@MichaelBelgium
Copy link
Author

MichaelBelgium commented Feb 22, 2024

@karmendra Yeah correct, wsl-init expects mysql already being installed before running the file itself

apt-get remove -y --purge mysql-server mysql-client mysql-common

I changed that in this PR to execute the mariadb.sh feature bash file but it's in there too

apt-get -o Dpkg::Options::="--force-confnew" remove -y --purge mysql-server mysql-client

I assume the command will fail but provisioning will not

We maintain the current behavior in wsl-init by including MySQL installation.
We offer an option to install MySQL later by adding a script to remove MariaDB and install MySQL 8.
What are your thoughts?

These separate feature (bash) files are extremely useful for WSL provisioning to be honest.

Eg I saw that the mailpit feature got removed (#1933) because - just like mysql - it's included in the vagrant base box, but what if i want mailpit in WSL? I can't because the script is gone. (Ofcourse i can manually grab said mailpit script, and it would work tho after changing homestead yml)

Perhaps there needs to be a decision whether or not the included base box features should be a seperate feature script (again). Just because homestead users are not only using vagrant now.

My initial thought is putting mysql as a feature/script, calling that bash file from wsl-init as default database engine. Or hear me out .... perhaps asking the user which database technology in the wsl-init script and then running the mysql or mariadb script accordingly

The behaviour to remove mysql or mariadb could be included in the mariadb or mysql file. We can check if mysql feature or mariadb feature is installed and if so, remove it

Like so:

if [ -f /home/$WSL_USER_NAME/.homestead-features/mysql]; then
    apt-get -o Dpkg::Options::="--force-confnew" remove -y --purge mysql-server mysql-client
    apt-get autoremove -y
    apt-get autoclean

    rm -rf /var/lib/mysql/*
    rm -rf /var/log/mysql
    rm -rf /etc/mysql
fi

@karmendra
Copy link

karmendra commented Feb 22, 2024

I believe MySQL should be included in the wsl-init script, as it's part of the default software list in the documentation. We can also add a feature script (mysql.sh) for users who wish to switch from MariaDB back to MySQL.

If you refer to the settlers/amd64.sh script, you'll notice that MySQL is installed by default.

According to the Laravel documentation for Homestead, the list of default packages installed in Homestead is outlined here.

Currently, WSL does not install the following software, and users need to be aware of it. We should provide an option to install these as features eventually by writing feature installation scripts:

Docker-cli
php5.6
php7.0
php7.1
php7.2
php7.3
php7.4
php8.0
php8.1
php8.2
Apache
wp-cli
gulp-cli
bower
yarn
grunt-cli
Beanstalk
Golang
Mailpit
ngrok
Postfix

Taking all this into consideration, I will update the wsl-init script to include MySQL by default, instead of MariaDB, and also create a feature script to install MySQL later.

@karmendra
Copy link

@svpernova09
Hello Joe, what are your thoughts regarding this PR?

@svpernova09
Copy link
Contributor

@svpernova09 Hello Joe, what are your thoughts regarding this PR?

Apologies I've not had time to get to this. ETA would be late next week.

@karmendra
Copy link

No problem at all, it's great to hear from you.
My PR will be ready for review by next week as well.
Thanks, and have a fantastic time ahead!

@MichaelBelgium
Copy link
Author

@karmendra

I believe MySQL should be included in the wsl-init script, as it's part of the default software list in the documentation. We can also add a feature script (mysql.sh) for users who wish to switch from MariaDB back to MySQL.

Fine for me! Creating the feature script and let wsl init call that script will be the cleanest

If you refer to the settlers/amd64.sh script, you'll notice that MySQL is installed by default.

According to the Laravel documentation for Homestead, the list of default packages installed in Homestead is outlined here.

What we're doing about mysql <> mariadb, we're gonna have to do the same with apache <> nginx no?
I see that the settler script installs apache by default too, but the docs say apache is optional.
And nginx is being installed also? Why would anyone want 2 kind of webservers locally?

https://github.com/laravel/settler/blob/be20e1f5961167bf4ba6daa2371e93facaccef03/scripts/amd64.sh#L549

https://github.com/laravel/settler/blob/be20e1f5961167bf4ba6daa2371e93facaccef03/scripts/amd64.sh#L577

Also in my opinion, there's way too much default software being installed, In my case i'd have to uninstall a lot more than installing if the WSL init script follows the settler script. I like how minimal my WSL environment is right now

I would at least propose to not default install

  • EOL php versions (< 8.1)
  • multiple database engines: why are 3 database engines being installed (mysql, sqlite3 and postgres)? I'd only keep mysql and let the user choose via features if they want more

Currently, WSL does not install the following software, and users need to be aware of it. We should provide an option to install these as features eventually by writing feature installation scripts

Agree, for most of them there is already a script

@karmendra
Copy link

Hi @MichaelBelgium,

I have created a Draft PR, #1956

Have a look at it. Let me know your opinion.

One thing I didn't make any changes in wsl-init, instead created new file wsl-init.sh. But in the final PR i want to replace wsl-init with wsl-init.sh

Thanks,
Karmendra

@svpernova09
Copy link
Contributor

Thanks for the ton of work that's gone into this.

RE: MySQL or Maria DB? We should follow Settler's lead in a build-time configuration that users can override to get exactly what they want.

The purpose of WSL-INIT is to be a lightweight alternative. I don't want every PHP version installed typically, this should easily be a runtime option.

So we would have something at the top of the wsl-init that would be ~

INSTALL_PHP_83=true
INSTALL_PHP_82=false
INSTALL_PHP_81=false
INSTALL_PHP_80=false
INSTALL_PHP_74=false
INSTALL_PHP_73=false
INSTALL_PHP_72=false
INSTALL_PHP_71=false
INSTALL_PHP_70=false
INSTALL_PHP_56=false

INSTALL_MYSQL=false
INSTALL_MARIADB=true
INSTALL_POSTGRESQL=false

echo "### Build Configuration ###"
echo "ARCH               = ${ARCH}"
echo "INSTALL_PHP_83     = ${INSTALL_PHP_83}"
echo "INSTALL_PHP_82     = ${INSTALL_PHP_82}"
echo "INSTALL_PHP_81     = ${INSTALL_PHP_81}"
echo "INSTALL_PHP_80     = ${INSTALL_PHP_80}"
echo "INSTALL_PHP_74     = ${INSTALL_PHP_74}"
echo "INSTALL_PHP_73     = ${INSTALL_PHP_73}"
echo "INSTALL_PHP_72     = ${INSTALL_PHP_72}"
echo "INSTALL_PHP_71     = ${INSTALL_PHP_71}"
echo "INSTALL_PHP_70     = ${INSTALL_PHP_70}"
echo "INSTALL_PHP_56     = ${INSTALL_PHP_56}"
echo "INSTALL_MYSQL      = ${INSTALL_MYSQL}"
echo "INSTALL_MARIADB    = ${INSTALL_MARIADB}"
echo "INSTALL_POSTGRESQL = ${INSTALL_POSTGRESQL}"
echo "### Build Configuration ###"

Then we can wrap the installer scripts around the feature checks. Thoughts?

@karmendra
Copy link

karmendra commented Feb 27, 2024

Hi @svpernova09
Shelter's build-time configuration may not fit our current user workflow. Users are accustomed to modifying provisioning based on the YAML file rather than directly altering the wsl-init script, which could be complex for them.

I suggest maintaining wsl-init as lightweight as it is, with MySQL as the default database. Users can opt to install MariaDB if required, streamlining the setup process.

In my draft PR I have added the documentation for wsl installation, I have tried to maintain what I suggested above.

##Update: As a second thought, instead of following steps to provision wsl.

sudo ./bin/wsl-init
sudo ./bin/homestead wsl:features
sudo ./bin/homestead wsl:sites
sudo ./bin/homestead wsl:databases
sudo ./bin/homestead wsl:folders

we can change the steps to following:

bash init.sh
- Now modify the Homestead.yaml to decide the features to be installed. at this point they decide to install the versions of php, database to be installed. wsl-init should be able to read the yaml file for this information.

sudo ./bin/wsl-init
sudo ./bin/homestead wsl:features
sudo ./bin/homestead wsl:sites
sudo ./bin/homestead wsl:databases
sudo ./bin/homestead wsl:folders

@MichaelBelgium
Copy link
Author

Hi @MichaelBelgium,

I have created a Draft PR, #1956

Have a look at it. Let me know your opinion.

One thing I didn't make any changes in wsl-init, instead created new file wsl-init.sh. But in the final PR i want to replace wsl-init with wsl-init.sh

Thanks, Karmendra

Cool, will check it out later!

Thanks for the ton of work that's gone into this.

RE: MySQL or Maria DB? We should follow Settler's lead in a build-time configuration that users can override to get exactly what they want.

The purpose of WSL-INIT is to be a lightweight alternative. I don't want every PHP version installed typically, this should easily be a runtime option.

So we would have something at the top of the wsl-init that would be ~

INSTALL_PHP_83=true
INSTALL_PHP_82=false
INSTALL_PHP_81=false
INSTALL_PHP_80=false
INSTALL_PHP_74=false
INSTALL_PHP_73=false
INSTALL_PHP_72=false
INSTALL_PHP_71=false
INSTALL_PHP_70=false
INSTALL_PHP_56=false

INSTALL_MYSQL=false
INSTALL_MARIADB=true
INSTALL_POSTGRESQL=false

echo "### Build Configuration ###"
echo "ARCH               = ${ARCH}"
echo "INSTALL_PHP_83     = ${INSTALL_PHP_83}"
echo "INSTALL_PHP_82     = ${INSTALL_PHP_82}"
echo "INSTALL_PHP_81     = ${INSTALL_PHP_81}"
echo "INSTALL_PHP_80     = ${INSTALL_PHP_80}"
echo "INSTALL_PHP_74     = ${INSTALL_PHP_74}"
echo "INSTALL_PHP_73     = ${INSTALL_PHP_73}"
echo "INSTALL_PHP_72     = ${INSTALL_PHP_72}"
echo "INSTALL_PHP_71     = ${INSTALL_PHP_71}"
echo "INSTALL_PHP_70     = ${INSTALL_PHP_70}"
echo "INSTALL_PHP_56     = ${INSTALL_PHP_56}"
echo "INSTALL_MYSQL      = ${INSTALL_MYSQL}"
echo "INSTALL_MARIADB    = ${INSTALL_MARIADB}"
echo "INSTALL_POSTGRESQL = ${INSTALL_POSTGRESQL}"
echo "### Build Configuration ###"

Then we can wrap the installer scripts around the feature checks. Thoughts?

Those script variables look the same as setting the feature to true in homestead.yml.
If thats the case, a WSL user should edit homestead.yml and wsl-init before deploying ?
I'd keep it with one file though

In my opinion, the only things that are located in wsl-init should be things that are going to be installed by default. Eg mysql and php 8.3 at least. No questions asked (other than wsl user and group), just execute the thing.

Execute, boom, you got 8.3 and mysql on your wsl instance. Then if the user wants more php versions or other database engine, edit the features in homestead.yaml

I feel like this approach would be easier?

Ok random thought with the deploy variables, if you want to make it a choice, whether or not default things get installed, might aswell make a feature script for them too and on deploy don't install php 8.3 / mysql but let the user decide it in homstead.yaml

I don't know, there's a lot of approaches 😅
My focus is on re-usability of the feature scripts, quick deploy and use advantage of the /home/$WSL_USER_NAME/.homestead-features folder to check for installed features to install/uninstall other features

@karmendra
Copy link

One concern I've noticed is the lack of uninstall scripts for software other than MySQL and MariaDB.

Presently, when MariaDB is set to true in the YAML configuration, it uninstalls MySQL and installs itself, as they cannot run simultaneously. Similarly, in my pull request where I added the MySQL feature script, I followed the same approach: uninstalling MariaDB before installing MySQL.

Do we need to have uninstall scripts to remove features if user changes feature from true to false?

@svpernova09
Copy link
Contributor

Do we need to have uninstall scripts to remove features if user changes feature from true to false?

No, not at all. This is a weird situation for the WSL because in Vagrant-land you would just delete the VM and re-provision. This isn't nearly as possible in WSL. In my opinion feature uninstallation is going to be a nightmare and likely leave a mess of things. I don't want to debug a 4th or 5th set of installations and uninstallations.

@svpernova09
Copy link
Contributor

One concern I've noticed is the lack of uninstall scripts for software other than MySQL and MariaDB.

The only reason there is an uninstall of MySQL is because it can't coexist on the same system (easily) with MariaDB. To install MariaDB we have to purge MySQL.

@svpernova09
Copy link
Contributor

Merging this into a wsl-rework branch for testing and 🔨-ing

@svpernova09 svpernova09 merged commit f180742 into laravel:wsl-rework Feb 28, 2024
4 checks passed
@svpernova09 svpernova09 mentioned this pull request Feb 28, 2024
2 tasks
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.

3 participants