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

dist/tools/esptools: add macOS support to install/export scripts #18385

Merged
merged 3 commits into from
Oct 13, 2022

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Jul 29, 2022

Contribution description

This PR provides the folling changes in dist/tools/esptools/{install,export}.sh to support macOS:

  • Since macOS doesn't have ldconfig command, the test for libncursesw.so version using the ldconfig command is moved to function install_qemu.
  • Since the only platform support by qemu-esp32 is linux-amd64, a platform test is added to the export_qemu function.
  • Platform Darwin-x86_64 added to the installation script.

The problem was found in https://forum.riot-os.org/t/esp32-undefined-reference-to-esp-mesh-is-scan-allowed/3680

Please note: I don't know whether the changes really work on macOS because of missing hardware.

Testing procedure

  • dist/tools/esptools/{install,export}.sh all should work as before on Linux.
  • dist/tools/esptools/{install,export}.sh all should now work on macOS as well.

Issues/PRs references

Since macOS doesn't have `ldconfig` command, it must not be used outside the `install_qemu` and `export_qemu` function.
Since the only platform support by `qemu-esp32` is `linux-amd64`, a platform test is added to the `export_qemu` function.
@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Jul 29, 2022
@gschorcht gschorcht added CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs CI: full build disable CI build filter Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Jul 29, 2022
@gschorcht
Copy link
Contributor Author

@benpicco Do you know who has Apple macOS and could test it?

@br0kenpixel
Copy link

I'd recommend changing the first if-statement in install_qemu to something like

if [[ ${OS} != "linux-amd64" && ${OS} != "macos" ]]; then
    echo "error: QEMU for ESP32 does not support OS ${OS}"
    exit 1
fi

Otherwise, it will block the rest of the function from working properly.
And the second one should check the version of ncurses/ncursesw based on the OS, something like:

if [ ${OS} != "macos" ]; then
    # qemu version depends on the version of ncurses lib
    if [ "$(ldconfig -p | grep libncursesw.so.6)" != "" ]; then
        ESP32_QEMU_VERSION="esp-develop-20220203"
    else
        ESP32_QEMU_VERSION="esp-develop-20210220"
    fi
else
    # qemu version depends on the version of ncurses lib
    if [[ # Some other method # ]]; then
        ESP32_QEMU_VERSION="esp-develop-20220203"
    else
        ESP32_QEMU_VERSION="esp-develop-20210220"
    fi
fi

(Please note that I'm not familiar with bash scripting)

@gschorcht
Copy link
Contributor Author

I'd recommend changing the first if-statement in install_qemu to something like

if [[ ${OS} != "linux-amd64" && ${OS} != "macos" ]]; then
    echo "error: QEMU for ESP32 does not support OS ${OS}"
    exit 1
fi

Hm, the precompiled qemu-esp32 is only avialble for one platform which is linux-amd64. AFAIK, linux-amd64 it is not working on macOS, correct me if I'm wrong.

@gschorcht
Copy link
Contributor Author

(Please note that I'm not familiar with bash scripting)

It's not Bash syntax. Used interpreter is just bin/sh, the Bourne Shell. Even though bin/sh is linked to the Dash on Debian-based Systems, the Dash is compliant with the Single UNIX Specification, that is, compliant with the Bourne Shell. So the script shouldn't require the Bash and should work on all UNIX compliant systems.

@gschorcht gschorcht added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: full build disable CI build filter labels Jul 30, 2022
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK. I do not have a macOS machine to test this. But few RIOT developers have. So I guess we can only provide best effort support for macOs only.

@maribu maribu enabled auto-merge October 12, 2022 21:05
@maribu maribu disabled auto-merge October 13, 2022 06:48
@maribu
Copy link
Member

maribu commented Oct 13, 2022

@leandrolanzieri: Does it make sense to rush this into the release? I doubt that testing of this will get any better by delaying it and it is a bug fix, so I'd say yes. What do you think?

@leandrolanzieri leandrolanzieri added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Oct 13, 2022
@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 13, 2022
@maribu maribu merged commit b0bccd9 into RIOT-OS:master Oct 13, 2022
@benpicco benpicco added Type: new feature The issue requests / The PR implemements a new feature for RIOT and removed Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 13, 2022
@gschorcht
Copy link
Contributor Author

@maribu Thanks.

@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
@gschorcht gschorcht deleted the dist/tools/esptool/install.sh branch October 22, 2022 11:38
bors bot added a commit that referenced this pull request Jan 11, 2023
19125: dist/tools/esptools/export.sh: fixes IDF_TOOLS_PATH default setting r=benpicco a=gschorcht

### Contribution description

This PR re-adds the setting of the default value of the variable `IDF_TOOLS_PATH` which was accidentally removed with PR #18385.

The `IDF_TOOLS_PATH` variable is used by the ESP-IDF to define the path where the Espressif toolchain is installed. Reusing this variable to set the path of the toolchain in RIOT allows the use of an existing Espressif toolchain if it has already been installed with ESP-IDF. If the variable is not defined it is set to `${HOME}/.espressif` as described in [documentation](https://doc.riot-os.org/group__cpu__esp32.html#esp32_local_toolchain_installation):
```
if [ -z ${IDF_TOOLS_PATH} ]; then
    IDF_TOOLS_PATH=${HOME}/.espressif
fi
```
Unfortunatly were these 3 lines be removed in `dist/tools/esptools/export.sh` accidentially with #18385 so that it became necessary to set this variable manually. If the variable was not set, the `dist/tools/esptools/export.sh` script didn't work for the Espressif toolchain installed with the companion script `dist/tools/esptools/install.sh`.

### Testing procedure

```
dist/tools/esptools/install.sh esp32
. dist/tools/esptools/export.sh esp32
```
Without the PR, the export doesn't work, with the PR it works.

### Issues/PRs references

Fixes PR #18385 


Co-authored-by: Gunar Schorcht <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants