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 default build script, minor docker fixes #57

Merged
merged 6 commits into from
Dec 14, 2024

Conversation

andy5995
Copy link
Contributor

@andy5995 andy5995 commented Dec 10, 2024

These commits should not necessarily be squashed when merging. Your call.

The PR title could be changed though.

The changes here I think are fairly self-explanatory, with some details in the first commit. It'll probably make more sense if you first look at the two commits separately, rather than from the "files changed" tab.

@andy5995 andy5995 force-pushed the docker/another-shell-fix branch 3 times, most recently from 2fb57c1 to 4868b0b Compare December 10, 2024 11:16
@andy5995 andy5995 marked this pull request as draft December 10, 2024 11:16
@andy5995 andy5995 force-pushed the docker/another-shell-fix branch 8 times, most recently from a9aa1f0 to a8fb063 Compare December 10, 2024 16:28
@andy5995 andy5995 marked this pull request as ready for review December 10, 2024 16:29
@andy5995 andy5995 changed the title Docker/another shell fix Add default build script, minor docker fixes Dec 10, 2024
@andy5995
Copy link
Contributor Author

@KaruroChori This is ready for review.

@KaruroChori
Copy link
Owner

KaruroChori commented Dec 10, 2024

Hey, thanks! It also cleaned up the yml file quite a bit.
I will add a note about that script in the dev docs as well, as it can simplify the initial (annoying) chain of commands.

scripts/build-default.sh Outdated Show resolved Hide resolved
@andy5995
Copy link
Contributor Author

Hey, thanks! It also cleaned up the yml file quite a bit. I will add a note about that script in the dev docs as well, as it can simplify the initial (annoying) chain of commands.

One problem with that script is that it takes longer because it reconfigures each time. I opened a related issue for that: #59

@KaruroChori
Copy link
Owner

KaruroChori commented Dec 10, 2024

There is a second problem I have been thinking. Meson subprojects should be probably updated at some point?

@andy5995
Copy link
Contributor Author

There is a second problem I have been thinking. Meson subprojects should be probably updated at some point?

Hmmm... locally yes. That could be added to this build script for now, and see how we like it, change later if necessary. Could add a check so it doesn't bother to run if it's in the CI.

We could add a prompt to the script, so the user can just hit y or n to run it. Maybe a timeout.. I think that can be done with 'read'

ChatGPT says yes, and I think I've experimented with that before:

#!/bin/bash

read -t 4 -p "Do you want to proceed? (yes/no) " answer

# Check if input was received within 4 seconds
if [ $? -ne 0 ]; then
    echo "Timed out. Defaulting to 'no'."
    answer="no"
fi

# Normalize input (convert to lowercase) and handle response
case "${answer,,}" in
    yes)
        echo "You chose yes."
        ;;
    no)
        echo "You chose no."
        ;;
    *)
        echo "Invalid input. Defaulting to 'no'."
        ;;
esac

* WORKSPACE did not get set correctly because it was doublequoted
* Set vars in yml
@andy5995 andy5995 force-pushed the docker/another-shell-fix branch from a8fb063 to a3d1125 Compare December 11, 2024 10:35
@andy5995 andy5995 force-pushed the docker/another-shell-fix branch from e5e5359 to a0e8aaa Compare December 11, 2024 16:43
fi

set -v
if [ "CI" != "true" ]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this section to update subprojects as a separate commit. Let me know if you hate it and I'll revert it. ;) I liked this idea better than the prompt w/timeout.

@KaruroChori KaruroChori merged commit ab48b20 into KaruroChori:master Dec 14, 2024
5 checks passed
@KaruroChori
Copy link
Owner

Thank you!

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.

2 participants