Skip to content

Conversation

@mdqst
Copy link

@mdqst mdqst commented Sep 22, 2025

switched the shebang to bash, added SCRIPT_DIR + REPO_ROOT, and updated docker build to always use the repo root.
now the script works no matter where you run it from.

Copy link
Member

@atanmarko atanmarko left a comment

Choose a reason for hiding this comment

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

Hello @mdqst, thank you for your PR. Unfortunately, we do not have yet the approval from our legal team to accept external contributions.

@mdqst
Copy link
Author

mdqst commented Sep 22, 2025

Hi atanmarko

ty for the update. I completely understand the need to wait for legal approval before accepting external contributions.

wanted to briefly highlight that this PR improves the script so it now works reliably no matter where it is run from. specifically, it:

  • switches the shebang to bash,
  • adds SCRIPT_DIR + REPO_ROOT,
  • updates the Docker build to always use the repo root.

this ensures a more robust and consistent workflow for anyone using the script. please let me know if there’s any additional information or clarification I can provide to help the legal team evaluate it, I’m happy to assist in any way.

ty for your time and consideration.

@Ekleog-Polygon Ekleog-Polygon removed their request for review September 22, 2025 15:35
@polykeith polykeith removed their request for review September 24, 2025 08:54
@mdqst mdqst requested a review from atanmarko September 28, 2025 11:00
@atanmarko atanmarko removed their request for review September 29, 2025 09:18

echo "Building the docker images. Note this script should be run from the repository root."
docker build -t aggkit-prover:local .
SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if something like the following would be enough:

SCRIPT_DIR="$(dirname "$0")"

It might treat symlinks a bit differently but is it an issue?

Also, the echo message saying the script should run from the root will be outdated with this change.

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