Skip to content

feat: add py-multiaddr from git #766

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

Merged
merged 8 commits into from
Jul 21, 2025
Merged

feat: add py-multiaddr from git #766

merged 8 commits into from
Jul 21, 2025

Conversation

acul71
Copy link
Contributor

@acul71 acul71 commented Jul 13, 2025

Modified pyproject.toml to use last git version of py-multiaddr ( Until we can publish in PyPi)

image

@acul71
Copy link
Contributor Author

acul71 commented Jul 13, 2025

@seetadev
Ready for review.
The failing test and typecheck error has been fixed in
#764

@seetadev
Copy link
Contributor

@acul71 : Thank you so much Luca. Appreciate your initiative and great efforts.

Wish if you could resolve the CI/CD issues and also add a news fragment file.

Will do a final review + merge at the earliest.

@acul71
Copy link
Contributor Author

acul71 commented Jul 17, 2025

Wish if you could resolve the CI/CD issues

It's happening in windows and right now (I will in a week) I don't have the right environment to test it, I'll do my best.
It has something to do with the py-multiaddr with the sub package py-cid-fork (my fork) in windows

@seetadev
Copy link
Contributor

@sumanjeet0012 and @Winter-Soren : Please collaborate with @acul71 on Windows testing.

@sumanjeet0012
Copy link
Contributor

@acul71

I have analyzed the errors and found that the issue occurs while opening the README.rst and HISTORY.rst files of setup.py in py-cid

The problem is that the file encoding is not explicitly specified, so the system defaults to a different encoding—most likely CP1252. This causes a decoding error during file reading.

To resolve this, we should explicitly specify the utf-8 encoding while opening the files.

Current Code:

with open('README.rst') as readme_file:
    readme = readme_file.read()

with open('HISTORY.rst') as history_file:
    history = history_file.read()

Recommended Fix:

with open('README.rst', encoding='utf-8') as readme_file:
    readme = readme_file.read()

with open('HISTORY.rst', encoding='utf-8') as history_file:
    history = history_file.read()

@sumanjeet0012
Copy link
Contributor

@acul71

We can also set environment variables to force UTF-8 decoding by configuring the following in the tox.ini file:

setenv =
    PYTHONIOENCODING = utf-8
    PYTHONUTF8 = 1

Try these approaches and share the results.
Let me know if these settings help in resolving the errors.

@acul71
Copy link
Contributor Author

acul71 commented Jul 19, 2025

Recommended Fix:

with open('README.rst', encoding='utf-8') as readme_file:
    readme = readme_file.read()

with open('HISTORY.rst', encoding='utf-8') as history_file:
    history = history_file.read()

@sumanjeet0012
This fix the issue.
Thank you man, I'm happy.
Ciao

@acul71
Copy link
Contributor Author

acul71 commented Jul 19, 2025

@seetadev
Many thanks to @sumanjeet0012 this #766 (comment) fixed the issue
Ready to be merged.
Just on doubt:
In pyproject.toml this uses the rolling version of py-multiaddr.
Should I point to a fixed commit, or keep it rolling?

multiaddr @ git+https://github.com/multiformats/py-multiaddr.git

@seetadev
Copy link
Contributor

@acul71 : Thanks a lot, Luca — appreciate the fix, and great work debugging and resolving the issue! The PR looks good and is definitely ready to be merged. Also, thank you so much @sumanjeet0012 for your continued support and efforts.

Regarding your question:

Should I point to a fixed commit, or keep it rolling?

Great point to bring up. While using a rolling version (@ main) is useful during early development and rapid iteration — especially when upstream changes are needed urgently — pinning to a specific commit hash is generally the safer approach for long-term stability and reproducibility. It ensures:

  • Builds won’t break due to upstream changes
  • CI behaves consistently over time
  • Dependencies are more deterministic for downstream users

So yes, I’d recommend pointing to a specific commit SHA in pyproject.toml, like this:

multiaddr = { git = "https://github.com/multiformats/py-multiaddr.git", rev = "abc1234" }

This way, we lock in the known-good version of py-multiaddr, and if we need to bump it later (e.g. after another upstream improvement), it’s a deliberate and reviewable step.

Once again — thanks for the contribution! Feel free to push the updated pyproject.toml change and we’ll get this merged.

@acul71
Copy link
Contributor Author

acul71 commented Jul 20, 2025

multiaddr = { git = "https://github.com/multiformats/py-multiaddr.git", rev = "abc1234" }

This failed with:

Obtaining file:///home/luca/Informatica/Learning/PNL_Launchpad_Curriculum/Libp2p/py-libp2p-fork
  Installing build dependencies ... done
  Checking if build backend supports build_editable ... done
  Getting requirements to build editable ... error
  error: subprocess-exited-with-error
  
  × Getting requirements to build editable did not run successfully.
  │ exit code: 1
  ╰─> [63 lines of output]
      configuration error: `project.dependencies[5]` must be string
      DESCRIPTION:
          Project dependency specification according to PEP 508
      
      GIVEN VALUE:
          {
              "git": "https://github.com/multiformats/py-multiaddr.git",
              "rev": "db8124e2321f316d3b7d2733c7df11d6ad9c03e6"
          }
      
      OFFENDING RULE: 'type'
      
      DEFINITION:
          {
              "$id": "#/definitions/dependency",
              "title": "Dependency",
              "type": "string",
              "format": "pep508"
          }

So I used string syntax:

"multiaddr @ git+https://github.com/multiformats/py-multiaddr.git@db8124e2321f316d3b7d2733c7df11d6ad9c03e6",

Ready to Merge,
Thanks Manu

@seetadev
Copy link
Contributor

@acul71 : Great, thank you for the wonderful contribution. Doing a final review. Will merge soon.

@seetadev
Copy link
Contributor

Wish to also thank @sumanjeet0012 for his support and contribution too.

@seetadev seetadev merged commit 8bf261c into libp2p:main Jul 21, 2025
28 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.

3 participants