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

Windows compatibility #1

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Windows compatibility #1

wants to merge 4 commits into from

Conversation

rspeir
Copy link

@rspeir rspeir commented Sep 2, 2022

Hi krzmaz,

I converted the perl script that generates the fsdata.c into a python script that doesn't rely on *nix shell commands. I also modified the cmakelists file slightly so that it uses the new python script and doesn't rely on the *nix mv command to rename the generated fsdata.c file to my_fsdata.c.

The project will build under Windows now, and I tested it on an Ubuntu install to verify it still works under Linux. I don't have a MacOS install to test it on, but I don't see why it wouldn't work.

Please excuse the ugliness of my python script; it's probably not the most "pythonic" way of doing things, as I was just doing a dirty line-by-line conversion of the perl script your project pulled in.

… modified cmakelists file to not use *nix shell commands. Project now builds under Windows as well as Ubuntu (MacOS not tested but should work)
Copy link
Owner

@krzmaz krzmaz left a comment

Choose a reason for hiding this comment

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

Hi @rspeir, sorry for the late response, I was on vacation.
Thank you for working on Windows compatibility! It's great to see the repository finding some users! 😄
Let me start with a suggestion to move your python script to a place where it is easier for other people to use it not only in Rpi pico context. You could create a separate repository for it, an Github gist, or an fork of the lwIP repository from which you could create a pull request for it. I think it could be helpful to people that share your preference of Python over Perl.
However I don't think that it should go into this repository for multiple reasons:

  1. As stated above, its use is not limited to this repository, it could be an external dependency like the original makefsdata script.
  2. Its maintenance/further development should be separate from the version history of this repository
  3. I think we should extend the Perl script, rather than reinventing it. AFAIK Python3 is also not installed on Windows by default. Therefore I see no difference in depending on having Python3 interpreter over depending on an Perl interpreter. Especially that according to https://www.perl.org/get.html, Perl is installed by default on 2 out of 3 major platforms, whereas AFAIK it is not a given for Python3 to be present on every linux distro.

In general my preference would be to stick with the Perl implementation as much as possible, as it is maintained together with the code that uses fsdata.c. Since you already put the work into creating the Python script, I'm open to introducing it as a "best effort backup", but still recommending to install Perl if both Python3 and Perl aren't found on the system.

Steps that I would propose:

  • create another PR for the original Perl script to make it Windows friendly (preferably on top of my PR)
  • extract your Python script to a more generally available location as suggested above
  • add logic in CMake to:
    1. Check for Perl and if it is available, use the original makefsdata script (the forked version until the PR gets merged)
    2. If Perl isn't available, check for Python3, and if it is available print a warning about it and fall back to your external Python script
    3. If both Perl and Python3 aren't available print a fatal error suggesting to install Perl
  • add the RENAME in CMake to make it work on Windows. (thanks again for showing me that! 😁 )

Let me know what do you think about my proposal :)

@@ -1,4 +1,5 @@
build
.vscode
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not fully on board with adding vscode specific ignores. The idea was for this repository to be a template - if an user will use CLion IDE, they don't need this entry, but need other, CLion specific ones.
We could either keep the gitignore as brief as possible and let users extend it as they need, or we could try to pull in as broad gitignore as possible, ignoring stuff from vscode, CLion, clangd, vim, emacs, etc...

set(MAKE_FS_DATA_SCRIPT ${CMAKE_CURRENT_LIST_DIR}/external/makefsdata)
set(MAKE_FS_DATA_SCRIPT ${CMAKE_CURRENT_LIST_DIR}/makefsdata.py)

find_package(Python3 COMPONENTS Interpreter REQUIRED)
Copy link
Owner

Choose a reason for hiding this comment

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

I like the approach of using CMake to find the interpreter executable.
IMO if we introduce this kind of mechanism we should also introduce error handling for when Python is not found on the system.

RESULT_VARIABLE FSDATA_RESULT
)

file(RENAME fsdata.c my_fsdata.c)
Copy link
Owner

Choose a reason for hiding this comment

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

This is awesome, I wasn't aware of RENAME functionality!
This should definitely go in.

@rspeir
Copy link
Author

rspeir commented Sep 25, 2022

Hi krzmaz,

It's my turn to apologize for the long delay. I got super busy with work and had a bout with Covid. I have read your comments and made a few modifications to the cmakelists file that I think offer a good compromise:

1. As stated above, its use is not limited to this repository, it could be an external dependency like the original `makefsdata` script.
2. Its maintenance/further development should be separate from the version history of this repository

I have separated my python script into its own gist on my github page.

3. I think we should extend the Perl script, rather than reinventing it. AFAIK Python3 is also not installed on Windows by default. Therefore I see no difference in depending on having Python3 interpreter over depending on an Perl interpreter. Especially that according to https://www.perl.org/get.html, Perl is installed by default on 2 out of 3 major platforms, whereas AFAIK it is not a given for Python3 to be present on every linux distro.

I installed a perl interpreter on my PC to see if the script would work without modification in a Windows environment, but it still utilizes certain shell commands that work differently on Windows than a *nix environment, so even with a perl interpreter installed, it won't work under Windows.

You are correct that Python 3 is not installed by default on Windows, but many of the installation scripts that install the Raspberry Pi Pico dev tools also install it, so I thought it would be more likely that someone using this repo would also have Python 3.

Modifying the original perl script is probably the "cleanest" option, but I have never particularly liked the language and don't know enough about it to make changes to the script.

Steps that I would propose:

* create another PR for the original Perl script to make it Windows friendly (preferably on top of [my PR](https://github.com/lwip-tcpip/lwip/pull/15))

* extract your Python script to a more generally available location as suggested above

* add logic in CMake to:
  
  1. Check for Perl and if it is available, use the original `makefsdata` script (the forked version until the PR gets merged)
  2. If Perl isn't available, check for Python3, and if it is available print a warning about it and fall back to your external Python script
  3. If both Perl and Python3 aren't available print a fatal error suggesting to install Perl

Here's my solution:

  1. Check what OS cmake is running under
  2. If it's running under Windows (and the original perl script won't work at all) then pull in the python version of makefsdata
  3. If it's not running under Windows, default to using the perl version of the script.

@krzmaz
Copy link
Owner

krzmaz commented Oct 3, 2022

@rspeir sounds good, can you rebase your changes accordingly?

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