olliw42 / mLRS

2.4 GHz & 915/868 MHz & 433 MHz/70 cm LoRa based telemetry and radio link for remote controlled vehicles
GNU General Public License v3.0
295 stars 67 forks source link

Add Linux support to some python scripts #103

Closed brad112358 closed 1 year ago

brad112358 commented 1 year ago

Note that I commented out the code which copies the license.md files. The case of these file names is inconsistent in the source path, so this code didn't always work on Linux. This is clearly not needed for building so I'm not sure why you bothered when you didn't copy other documentation files. Let me know if you think it is needed.

olliw42 commented 1 year ago

many thx for this! I think few points need to be looked at.

also some inline comments

:)

brad112358 commented 1 year ago

first off, can I assume this was also tested on Win, or should I do?

I should have mentioned no testing on Windows. Sorry, I only have one computer with windows installed, and it runs Linux 99% of the time. It also has no development tools installed as I don't do development on Windows.

run_make_firmware and run_replace_xxx are empty. Pl correct this. (merging this PR will kill them, won't it?)

No, I've only changed the modes on these files to make them executable on Linux. Github is misleading in this case. They are not empty in my branch.

I don't understand what "The case of these file names is inconsistent in the source path, so this code didn't always work on Linux" means

Some of the source paths you are copying have a file named "License.md" and some are named "LICENSE.md". By default Windows doesn't care about the case in file names, but Linux does.

In the places where I commented out the copy of these files, the folders where you are copying the license files are not completely empty. They obviously contain a folder since you are copying the files to a path that ends in "..". Are you sure you really need this file in these two cases?

run_make_firmware3: the path things I find very strange. Are these the canonical paths as any user would get them when they install STM32CubeIDE on thier linux system, or are these the paths which you made them to be on your system?

The IDE install package from ST installs itself in /opt/st. This is a common convention for third party Linux packages which don't use a specific Linux distribution's native package format and thus, aren't managed by the Linux system. I find "C:" strange also. :)

olliw42 commented 1 year ago

first off, can I assume this was also tested on Win, or should I do?

I should have mentioned no testing on Windows. Sorry, I only have one computer with windows installed, and it runs Linux 99% of the time. It also has no development tools installed as I don't do development on Windows.

ok, no problem. I will then.

run_make_firmware and run_replace_xxx are empty. Pl correct this. (merging this PR will kill them, won't it?)

No, I've only changed the modes on these files to make them executable on Linux. Github is misleading in this case. They are not empty in my branch.

not clear to me what was then changed. But, ok.

I don't understand what "The case of these file names is inconsistent in the source path, so this code didn't always work on Linux" means

Some of the source paths you are copying have a file named "License.md" and some are named "LICENSE.md". By default Windows doesn't care about the case in file names, but Linux does.

ah oh ah

In the places where I commented out the copy of these files, the folders where you are copying the license files are not completely empty. They obviously contain a folder since you are copying the files to a path that ends in "..". Are you sure you really need this file in these two cases?

you are right, I confused with the copy of LICENSE.txt in do_for_each_target(), which I found to be needed

anyway, I would prefer to copy them, for licence sakes correctness, so it is impossible to copy with Lunix, pl keep the lines for Win

run_make_firmware3: the path things I find very strange. Are these the canonical paths as any user would get them when they install STM32CubeIDE on thier linux system, or are these the paths which you made them to be on your system?

The IDE install package from ST installs itself in /opt/st. This is a common convention for third party Linux packages which don't use a specific Linux distribution's native package format and thus, aren't managed by the Linux system. I find "C:" strange also. :)

ok. I just wanted to be sure these two path manglings you've added are not just your special locations :)

Overall, I would wish there would be a simpler and less verbose and spilling way to make the scripts linux&win compatible. Maybe one could have one place in the top which separates out the two cases, but I guess for now it's what it is.

So, two points left:

:)

brad112358 commented 1 year ago

could you pl check if you can find a not too clunky line to copy the license mds, or to keep the line only for win

If you insist on having these files, I guess we can test which specific version of the file exists before copying it. Is that good enough?

could you pl see if it makes the firmware3 code look simpler by introducing md & mkdir function

Perhaps I don't understand what you are asking for here. At first I thought you didn't like the duplication of the code in create_clean_dir(), so I simplified it in my last commit by calling the existing functions. Is this not simple enough?

olliw42 commented 1 year ago

If you insist on having these files

for win I would insist, for linux I don't care :)

, I guess we can test which specific version of the file exists before copying it. Is that good enough?

I'm surprised that linux hasn't some kind of wildcard or whatever to make copy case insensitive, but, hey, what do I know about linux LOL

as said, if you find the resulting code to complex, just do the copy only for win

could you pl see if it makes the firmware3 code look simpler by introducing md & mkdir function

Perhaps I don't understand what you are asking for here.

you did already (I missed to see you did some changes already before I commented) :)

brad112358 commented 1 year ago

I'm surprised that linux hasn't some kind of wildcard or whatever to make copy case insensitive

There are ways of doing this from the shell on Linux by setting the nocaseglob option, but it's a little ugly and you gave me an easy way out so I'll take it. :)

olliw42 commented 1 year ago

THX! I will merge now. There might be some follow up changes, to see if one can't separate the difference better. Not sure yet. Anyways. GREAT and THX again.

brad112358 commented 1 year ago

Thanks!

olliw42 commented 1 year ago

@brad112358

I made some little changes, mostly cosmetics. Can you pl quickly double check if the ymake sense and if it's still all good for you?

all thanks to you :)