Skip to content

python312Packages.rns: fix script shebang to use python-env#337285

Closed
aos wants to merge 1 commit intoNixOS:masterfrom
aos:rns-python-wrap
Closed

python312Packages.rns: fix script shebang to use python-env#337285
aos wants to merge 1 commit intoNixOS:masterfrom
aos:rns-python-wrap

Conversation

@aos
Copy link
Member

@aos aos commented Aug 25, 2024

Description of changes

Closes #336766

The patch now fixes the generated script's shebang to use Python environment from nix.

Output should now be:

out.write("#!/nix/store/s96mvj6x6nbr523d489s5d72f9kv0q34-python3-3.12.4-env/bin/python3.12\n\n".encode("utf-8")+gzip.decompress(base64.b64decode(recovery_esptool)))

CC: @sgiath can you please confirm this works for you? I don't have a way to trigger the script to run.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Aug 25, 2024
@emilazy
Copy link
Member

emilazy commented Aug 25, 2024

Is this used internally for the build or for end users? If the former, the packages should be added to the build environment and patchShebangs used. If the latter, buildPythonApplication or makeWrapper are more appropriate.

Never mind, I guess this is a program that itself produces a script as output? Maybe this is the best way then? It’s weird though. It should at least be a proper .patch file.

@emilazy
Copy link
Member

emilazy commented Aug 25, 2024

Okay, after actually reading the linked issue (sorry!) you should instead replace the shebang with #!${python3.withPackages (ps: […])}.

@aos
Copy link
Member Author

aos commented Aug 25, 2024

@emilazy Yeah it's a super weird one - I decided to send this first and hope someone came up with a better solution :-) Lemme amend to use your suggestion. I didn't know you can use a bin like that to "wrap" a python with its packages. Thank you!

@aos aos force-pushed the rns-python-wrap branch from 2ba5bea to 8fdc8f5 Compare August 25, 2024 17:07
@aos aos changed the title rns: fix script shebang to use nix-shell rns: fix script shebang to use python-env Aug 25, 2024
@emilazy
Copy link
Member

emilazy commented Aug 25, 2024

Thanks, this looks correct now. Instead of a brittle line number reference, please make a .patch file that replaces the relevant shebang with #!@python@, and then use replaceVars to put it in patches:

  patches = [
    (replaceVars ./fix-generated-shebang.patch {
      python3 = lib.getExe (python3.withPackages);
    })
  ];

Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Can't we patch the code to use esptool provided by Nixpkgs rather than download it?

@emilazy
Copy link
Member

emilazy commented Aug 25, 2024

I agree that @dotlambda’s approach to use our own package would be better if it’s simple to do, but I don’t know enough about this program to say whether it is or not. Maybe you can just patch out the generation/downloading/whatever and add the path to our esptool.

@sgiath
Copy link
Contributor

sgiath commented Aug 25, 2024

I have asked what is the reasoning behind generating the code in the original repo markqvist/Reticulum#532

@ofborg ofborg bot requested a review from fabaff August 25, 2024 18:20
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Aug 25, 2024
@aos aos force-pushed the rns-python-wrap branch from 8fdc8f5 to 5efbcad Compare August 25, 2024 19:17
@aos
Copy link
Member Author

aos commented Aug 25, 2024

Cool - I didn't know about replaceVars, that's really handy. I took your suggestion @emilazy. And I also agree with you @dotlambda on this regard. I've made the change for now.

We can wait to see if the maintainers respond to @sgiath's issue and can take a different approach from there if you'd like.

@emilazy
Copy link
Member

emilazy commented Aug 25, 2024

Oh, wow, that Base64…

This seems like an improvement over the status quo, so if the PR title and commit message are updated I’m happy to merge. But we should really stop it doing this, upstream response or not; a quick diff of the decoded blob compared to our package should be enough to tell if there’s any good reason for this weird obfuscated code. Let’s hope it’s not backdoored or something 🫠

@emilazy
Copy link
Member

emilazy commented Aug 25, 2024

And all credit for replaceVars goes to @philiptaron for #301350 :) (We also had substituteAll already for a long time, but we’re moving over to replaceVars.)

@dotlambda dotlambda changed the title rns: fix script shebang to use python-env python312Packages.rns: fix script shebang to use python-env Aug 25, 2024
@dotlambda
Copy link
Member

if the PR title and commit message are updated I’m happy to merge

@aos In fact there is no package called rns.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
python = lib.getExe (python.withPackages (ps: [ ps.pyserial ]));
python = lib.getExe (python.withPackages [ pyserial ]);

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't we use the one that comes with the python environment through ps:? Or does it make a difference here?

Copy link
Member

Choose a reason for hiding this comment

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

python designates exactly the one that matches your environment, I think, so these should be equivalent?

Copy link
Member

Choose a reason for hiding this comment

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

It actually doesn't make a difference because any packageOverrides would be present in python.pkgs as well. So I guess either is fine.

Closes NixOS#336766

Fixes the generated script's shebang to use Python environment from nix.
@aos aos force-pushed the rns-python-wrap branch from 5efbcad to d555dbb Compare August 25, 2024 19:52
@aos
Copy link
Member Author

aos commented Aug 25, 2024

Fixed the commit message! Thanks @dotlambda for fixing the title.

Oh, wow, that Base64…

Yeah it's a lot. Maybe we can decode the base64 and patch in the script directly to be called?

@emilazy
Copy link
Member

emilazy commented Aug 25, 2024

The Base64 decodes to some obfuscated/minified/bundled‐looking thing that decodes more Base64 that doesn’t seem to be present in the upstream esptool repository, and then passes it to eval:

ESP32ROM.STUB_CODE=eval(zlib.decompress(base64.b64decode(b'\neNqVWm1z2zYS/iu0EtmRL+kAFEUCvutEdh3ZTtKp3Tayk1PvSoJgk7uMR3Z0Y9lN/vthX0CAlJrefZBNguBid7H77Av4+97Krld7B0m1t1gL5X5isW6y54u1NNENXLQ3ZbZY28rd1DAtPMkP4XLHXZfu1yzWRiQwAlRT96zRneEn7k+WJKvFWrulbOpuc/ebhNWEgLcm9JaS7n/eoeBYAdqOHaWI+xLGhCNpRRBHVIMGWHCjhZsKNDKgA5zKDkFN02TtRkUktUpY9EbFojrO4f26x5RjxnEAM5V4PD+lpziz/F9m9leHnxRJuxNJb0/wpzxHFtRlvHgVkRSGtBEWZkmRqypSsO5xqNP3dBFGUNXz+01RHMXPbjQFaQYiSWhrtokjxJT4tZ5ZN8/tiy4DK7aOFGf6bOmeQF2utq9Jmu6PCUlvG8EWDQT8DydkSc+8kRt1BDw/IqstQesqCGJKsmQFs0D7oFjchbEbdFZY8r1SA7c86w99yQ1KDazCn7EQKxsMB5cZs47wTUkKb5opk5BA370qWXVejQbojnmM1VnCddPfSL24ph2o5f+pdQPWjAIotjE9/pZecYpgIQ0+OkIBDpQehN3SItoFoaZ8pXobp7L4fjr1V6c0jO/orCXlHaOSfosSxgjQZlWwJG7TrN+0CGl0dN2Ci2bRVewXVTqKMIE3ye9sZ6YGiGFs0vwDRJLOEY0mxg2PtS+Z9IrfcFzqKsbA9KzvotEC6Epl4MYv5rWK1wXs7YwnZ0FyO4kMhc0b14+NwCCIVdGrSA/teEYwI8RnIgBPpCNg5QxtJdrWngEGverFqiXTHb/uvrUipuuIUUQlNMtIOazILkicPwNPZlhyfyrQTXOVnY8NuTRsqRi7d2X27qfzxeKQQgm9bTkiol8eO4XlvAMYmx6zz0/IWUGrdryJehDMJOx5TThR1SS2ap28G9dae1TmYEC3Jhv9/ASoHAxG8O9JBgSM0DEQq24EQQdaUgRuyuenj1ERMHdAKilDZKk9ctQEnCoC58Dat7AziAMpgY31WyHJOMs0WL8HPgxJkrRkZeR9abBB71a9AFriVRKPpx+8V+6wsiKsQ67FNoU+AszdkgBAKPDhQHhEATLKe0tKbJCEpzAB5DOHPoSmcQqAI3KkkMMMLGCwPxZ/O+QYkY6u9GkcVJ4hJINCS7/Rkz6XT4mJNrSCEniu4A1L2TVhmqVdgOd1xSqptqjEzzFs7uMubXzX01RMp/gKnZrnZJtzNqM2SXLgU8M0PEML4ntZDRiqUADmpsn+KJXy11fxjQOoGvF/CgjyDfsAoFg7DBkxyOtu8p0d4gFipGRb8FE6lsnp7Dpe/yIktOje8vw7wx6fRbskw7StHt+Y/fBWxc64wc8GZrwcTMdgD/Mxpa3oFLzrVfQ2QHRZdvOwDh9RPMA6oQrv4EYUZLVkLjmxF5SANmz+eMODuRgWxVR/Zi4f4818H98s45tVfLOOb0CpvzEe1qJ1JljvPbvVThky5DhblmVzRnJKBLoqaBL9OXu6uH4LhI4anhJlFUGki1CPoMw+MS7fQMSaXLo9Umz1ud+ImtbF+dvMr93AG3fRsgtMPlwgS7PdaCZu6fQT6V0yaPvaisxsufb2WnTt1civRijgz/FQN1FFMvkRI+DDDQpxG3n3xNv/ElK0JAQcjJ1oDMPgIp4diWle8ivE34S4qja0/DB4OC4Ido3lRBfpnK+2y60KkHRM+GfjwsCAb5fp7COTEbF+4cmToNt6I97MOTGzlDyVPsvE/f1IZKrKUkRC+yw4RwXDyF8sVu8ofy3TVx4I33B9Ow4+XTa8RkGZhlHNC2Dhx11YAjQBFXd6SSqBHAS8W6OOfwUFJqRYxAMWRm9ggg1Aga/n28DBhrwHozwa+FNfblTm6z6OKbF5/sPp4Rnx2fYgQNmQMohqShkUkoAbEZoYWE9kz3u1Xa8QxN0y3XpDimmncu2nrcgVIXF74xS2F1HIou6JTzw8Cx1JhIqIVKEe+vyel7aMUVc++5x+3MdYpVIOWdKhDV0ZQ1ev6R+kohMmA9iiSZw1xTpBtu1i21WLfa8pzgP2kX9j+Vvb1oWvByXCGuc+HkZsfyPhLQQfSeEUX2/DwhHEVPFqUKRs4xN6lxZ5RS5hZOI8vi7Y9em5rznsjr9w8aPGsLf/qN/VqdqSdoe9xLKB4O/dWcAyj1JBgkNCmab5kQwcOI/7WM6KzwchAx1x6i+/bMdkX6N0VGTyjf5YSltVleRKJptCvQwB1KSUHGIchQ4FdOUQKdjl4go+Xh6UVrF8teSek4kGEYwUqQdqglLGcBdHpg735L6HDN52fwxRMPuLT2lbsU4pK5D5qiPtFScLhmugoMfx0SHIfMS9QYmT9nBQ3l4sVqOLXSr4MQSY4o4oSMs2hgz6t8e3fIFdqmOgsTxOGrw4HXaYzM4uZt3MRZrHRxcL7hzUaYhkEPHJSAkrwYINbvHX170hx3FhbgnocEIoLdNuAoTUxiE6wDj4WBmN4xzcwFkUZMVtR2nohLqgH3KSU3wQ4k7Pm998ckGZABpLfpc0PNlwnkHjJ6DZBlEunf3GOsox4DUnOOiWQbhpLgNdg0g4e9xaFPaUYpZglbIglkCZzV0YN0SPKg7i3/cgcooVAZkbT3IWmjywXtVZb+ZhkNM37Te6x5XIz4ml/YbJIUu/+OmL1Z0OYAJ5oeXUqKvu44jd1qho3e5ytrW4k3j497aqQB/3zR6srNK+IqdBi9E0QVNk8fdoUPrBD8ScyLpRrp047kt0GVX1MCHrT5hz9wD1cesNfkam7crWs94z4xnMgtlg6ilGFz7QYEaYJtzCy3xTL4UQQbeWQ2e44GyIISv2VlEdW58pzrk0rX1e3P5gm4o5BFclt5VEmOaO2YDrfnqxS60C1CtS4r4xbuLREOWHv+MlYFkSenVVNiejbeyQPMOhxTFlKNhjK4mS7z63aTe0HpS82sg6D0C6Ww6lUByVnLg4VN8L1USbird9wjm1tBo7C/ZR8q/BtunFxmL7VJQr+Veua8Qd77A84PSelgczhQhuoh5DT5TTraLoTVHmrCfY+nzJMEim5beBsU+Z2DjZgQuGupL3DQ0K4DU3ES0h+VFLK/cdpyGJUOJFaskcOOVpm57byMS61/DAxY8hH0DYNPFSpCdkEfTKF15WvYGwPQz2BSGhFL51feutcx3ZswavyJ6evqAoL31c7/gjLlf65VCA9SQsuFxRlghAbiHoasuJAB6M1dQttnJUH2FCcP+Y6AJiqPEX1Ej+76h1rugtvyfOU1fxHubYNL0Gy1+/BeLDI4orDSS2mvvncfzU4gFOInwG46EK0l3VbyuVVA8BVGDdoqgtj+N4AiJO9qmzgnUaBKSJj/HcVYe5Uk7hIvGP8sjGKPqzaWftlIITA+zkiOOkFYZBD9BK4+kDlqI/cVaKiXCkLkzds02FrQhRtcr8uJpT0xVPhVKMTOufgdfXccZDc2IHkUUXoE1+yl6kYOfVfyC9fQMLvAArmbBZKr+3tgucjvR1a6Czoxhtd4/9Cs99rTTstcXFJkEM5E1UrpWgj16MmbOD2yl4wCGlcHry/eL6M3W00TJsZBl4nKWp0ATvg/2ANKHmNjjWcD1WNBem0Lcxcj3iHr/GAoAb1yU3Lky6s7jdJUMQNupl2/QZtrE+wzXgp0SoK+N+d6mhfSKrYzIAw0G/KjKqFER7zNAcB8jSapu/i9mjziYEfDKUbkEtxPiq1Zv1kA8GQYtYwth7uPsSMM1/SgCVBdYtNlgSpMmial2YegsinXIage3KW7hY0jKyn0uYfP42nNthFWL9I1K0246Tb19OaUxmsQ1g9HSs+vOKpj1peqD0VasPr+5w5WtmDWIWMGBgSmlOCrqV+YgSbzzZrajbhuV8/hAKBVk0FIFkcchxwa4ZOvBx65yjKFkoD4lsY5eEO2o8avkmZSz9dw8pNogs1fq4uXgCYsgwac7En1sMGXIh7QG7q3RbZfn41GA5NT1Bhyfjmd1jgl2HxLxAd9uNoiEeFFm69s7ozGDvjks1nPYJxoEyQi3KHz0X+QEWC46YL9Y9ptUpcvWDv83w9tOMqzLbS14Rg8BNsRV2NQyOrHyry/aKfeX5h0k1JxNRbeh9e7qCzMBDOlV2YnqCCdwxH5Ogos655cjApdNNtMCYNeaD0pbdTXDDTmskY42JBvCBF1mcg6jhkJAOU+7KhDcrH7/GR3FpCj1Hjl9VldA07L4zg3jwpOy/QhMHE2f7J4Uu5NV17OV4aCSO+XAVUj/IhUGpkhuVYAfaH6LF5z8abRwL4J/96dtOe6S1x0cpufduBN09AlrwIgDqcvJ6y/krvQyim8yLPqEkQIFD9DdD5i97J23gx/qSDzWLl9xI1iSUUsX0FKzljFNM7Y0s3ao8/0XHEk83jyJmUm4veJDGFCOJLF5eAuWbt6D1K/zGSN+cY+p9X653Pazc8YkCdgcOiFEsBdk7Sgxz9vtwPG3kzTnxXPrjWvkD95f5NArxiE+6scJSdOqHebUfM7426Wj0F8CzSu3CWjckfS1DFC3ZOA2e/u/6c18eoIBawuG74qMomCwqqhIx1E4S+p6Ab7HfuQxEsStaEojgwabXr6W6ueRmvPQfi/gTmGKL7ibv2DaZe5Lm3n+lAElAU681QU2pnFp1I89DBlvnIz4lwGCbEwyWEz4pkhl7FNpdcfZmFn0hA4ai21rmI+kZuIYBzc1BaJEECursavYhoIPmjlGYoM/ezho/4XQ4z4jVcASUbUE0mXgY/WbzqZKyPzj7R7sC2HrO3YSwyNM/g83dLRmmiBoa+SVro3N62H7P5xdqy4cn4ZstzHLwU6o0fN6gWKs+YRL4GdD4M/VOQxa5S9NFsTugrVC9SnfjVF2MHvl10WMHnnADbStac9Cezu+Fz7OINZz9jI+wtp3c1/5jPS9a3nl1EFjp6mrvaYIfjf7z06q8hU9HpSiySeqUmLkn9np1e98OyonM3WBdrsroG1M+4djjJzGhcS4mkyz78l9OLzRi')))

At the risk of sounding paranoid, I’m feeling very uneasy right now.

@emilazy
Copy link
Member

emilazy commented Aug 25, 2024

Okay, that just decodes to a benign‐looking Python dictionary literal. But we really need to rip this out. What is all this code and why is it being shipped in this form?

@emilazy
Copy link
Member

emilazy commented Aug 25, 2024

FWIW, in case anyone wants to take a look: https://gist.github.com/emilazy/47124ac58ddb0cde464e6435e12cd961

@sgiath
Copy link
Contributor

sgiath commented Aug 25, 2024

Serious Jia Tan vibes... I am interested in the author's rationale even more now

@emilazy
Copy link
Member

emilazy commented Aug 25, 2024

Okay, so the good news is that that scary ESP32ROM.STUB_CODE definition was present upstream (with some value; I didn’t check that the Base64 matches) as late as v3.3.1. So I think it is hopefully just a compressed, minified version of an old esptool.py. But if we can just make it call out directly to our current Nixpkgs esptool and it works, that would be far preferable.

@dotlambda
Copy link
Member

This would be how to use our own esptool: #337346
I haven't tested it though.

@sgiath
Copy link
Contributor

sgiath commented Aug 26, 2024

I have tried to build it and run it on my system and it seems to produce the old shebang. Maybe I am doing something wrong (this is my first time doing something like this)?

This is what I did:

git clone --depth 10 github.com:aos/nixpkgs aos-nixpkgs --single-branch --branch=rns-python-wrap 
cd aos-nixpkgs
nix-build -A python3Packages.rns  
 ./result/bin/rnodeconf --autoinstall

@dotlambda
Copy link
Member

I have tried to build it and run it on my system and it seems to produce the old shebang.

@sgiath Did you delete ~/.config/rnodeconf?

@sgiath
Copy link
Contributor

sgiath commented Aug 26, 2024

Yes, I did

@aos
Copy link
Member Author

aos commented Aug 26, 2024

Hmm... do you get the same error as before? Does it show the line: Extracting recovery ESP-Tool...? Maybe you need to make sure that the branch is up-to-date?

@sgiath
Copy link
Contributor

sgiath commented Aug 26, 2024

Yeah I am pretty sure. BTW it does override it if I edit the esptool.py manually to use the nix python so I cannot even test it that way

image
image
image

@dotlambda
Copy link
Member

image

Where does that license header come from?

@sgiath
Copy link
Contributor

sgiath commented Aug 26, 2024

Not sure... this is just what is on my system. Maybe it is a different file than the one we have been editing? :D it is all very confusing

@aos
Copy link
Member Author

aos commented Aug 26, 2024

If you delete the rnodeconf/update directory and re-run the built nix binary, does it recreate it with the same esptool.py?

@dotlambda
Copy link
Member

image

@sgiath Sorry to be pedantic but the screenshot doesn't include you deleting ~/.config/rnodeconf. Would you mind adding that?

@sgiath
Copy link
Contributor

sgiath commented Aug 26, 2024

Yeah I have deleted it 5 times 😅 I have also tried to edit the generated file to add nix shebang but it got overwritten.

image
image
image

@aos
Copy link
Member Author

aos commented Aug 26, 2024

@aos
Copy link
Member Author

aos commented Aug 26, 2024

And the recovery_esptool.py is there as a backup in case something gets hosed?

@sgiath
Copy link
Contributor

sgiath commented Aug 26, 2024

Looking at the folder there isn't recovery_esptool.py so you are probably right, I have tried to look through the code but it doesn't make sense to me. I cannot find a place where it gets generated.

image

@dotlambda
Copy link
Member

But the only line where ~/.config/rnodeconf/update/.../esptool.py is written is https://github.com/markqvist/Reticulum/blob/0.7.5/RNS/Utilities/rnodeconf.py#L2300.

@dotlambda
Copy link
Member

dotlambda commented Aug 26, 2024

Never mind. It downloads https://github.com/markqvist/RNode_Firmware/releases/download/1.72/rnode_firmware_lora32v21_tcxo.zip and extracts it here.
While we could add a line there patching the shebang of esptool.py if that's part of the zip archive I believe that #337346 is more correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RNS python package does not recognize dependencies

4 participants