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

Include both shell and ps1 entrypoint scripts with gleam export erlan… #4364

Merged
merged 4 commits into from
Mar 18, 2025

Conversation

Ummon
Copy link
Contributor

@Ummon Ummon commented Mar 16, 2025

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you! Could you update the changelog also please 🙏

It would be great to also update the CI workflow to test that both files to ensure they exist

- name: test/project_erlang export erlang-shipment (non-windows)
run: |
gleam export erlang-shipment
./build/erlang-shipment/entrypoint.sh run
working-directory: ./test/project_erlang
if: ${{ runner.os != 'Windows' && matrix.run-integration-tests }}
- name: test/project_erlang export erlang-shipment (windows)
run: |
gleam export erlang-shipment
.\build\erlang-shipment\entrypoint.ps1 run
working-directory: ./test/project_erlang_windows
if: ${{ runner.os == 'Windows' && matrix.run-integration-tests }}

static ENTRYPOINT_TEMPLATE: &str = include_str!("../templates/erlang-shipment-entrypoint.sh");
static ENTRYPOINT_TEMPLATE_POWERSHELL: &str =
include_str!("../templates/erlang-shipment-entrypoint.ps1");
static ENTRYPOINT_TEMPLATE_BOURNE_SHELL: &str =
Copy link
Member

Choose a reason for hiding this comment

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

It's not bourne shell, it's POSIX shell!

ENTRYPOINT_FILENAME_BOURNE_SHELL,
ENTRYPOINT_TEMPLATE_BOURNE_SHELL,
),
] {
Copy link
Member

Choose a reason for hiding this comment

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

Use functions for code reuse please, not loops

{entrypoint}
println!(
"
It can be copied to a compatible server with Erlang installed and run with one of the following scripts:
Copy link
Member

Choose a reason for hiding this comment

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

Wrap before 80 columns please.

"
It can be copied to a compatible server with Erlang installed and run with one of the following scripts:
- {ENTRYPOINT_FILENAME_POWERSHELL} (PowerShell script)
- {ENTRYPOINT_FILENAME_BOURNE_SHELL} (Bourne Shell script)
Copy link
Member

Choose a reason for hiding this comment

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

It's not bourne shell


{entrypoint}
println!(
Copy link
Member

Choose a reason for hiding this comment

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

Do not introduce an extra print statement please 🙏

@Ummon
Copy link
Contributor Author

Ummon commented Mar 17, 2025

For the CI workflow, should I replace the two steps by one like that (I'm not familiar with github workflows)?:

      - name: test/project_erlang export erlang-shipment
        run: |
          gleam export erlang-shipment
          .\build\erlang-shipment\entrypoint.ps1 run
          .\build\erlang-shipment\entrypoint.sh run
        working-directory: ./test/project_erlang_windows
        if: ${{ matrix.run-integration-tests }}

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!!

@lpil lpil enabled auto-merge (rebase) March 18, 2025 10:32
@lpil lpil merged commit a59aaca into gleam-lang:main Mar 18, 2025
12 checks passed
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.

None yet

2 participants