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

PEP 621 and refactorisation #203

Closed
wants to merge 9 commits into from
Closed

Conversation

n3rada
Copy link

@n3rada n3rada commented Oct 23, 2024

Hello to the maintainers of this great project! 👋

I had to use this project and I wanted to take advantage of it to help you comply with Python3 project standards. From now on, it will be easier and more consistent to install your tool. Cross-platform installation is easy and is handled by Python itself. Which could be interesting for a possible future use for a ROP chain on Windows, who knows the future!

I didn't take the time to refactor all the code to follow all the best practices, I just wanted to give it a quick polish. 🧼

If you're not comfortable with poetry, just ignore it. But you should know that publishing a package is very easy with this tool: https://python-poetry.org/docs/cli#publish

Hope it will suits your needs.

Best regards

@SweetVishnya
Copy link
Collaborator

Yeah, could you, please, remove the poetry part?

@SweetVishnya
Copy link
Collaborator

Btw, merging such large changes can cause another xz-utils) I need to find time to review all of this.

wget https://bootstrap.pypa.io/pip/2.7/get-pip.py
python2 get-pip.py
rm get-pip.py
python2 -m pip install --upgrade setuptools wheel importlib_metadata
Copy link
Collaborator

Choose a reason for hiding this comment

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

We still want to support Python2

@JonathanSalwan
Copy link
Owner

I didn't take the time to refactor all the code to follow all the best practices, I just wanted to give it a quick polish. 🧼

Is the quick polish has been automatically generated?

RUN="python3 ../ROPgadget.py"
fi

RUN="python3 -m ropgadget"
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should not run globally installed module


import ropgadget

ropgadget.main()
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you remove this script?

Copy link
Author

Choose a reason for hiding this comment

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

For compliance and modern python3 project structure. The goal is to fit the PEP 621 by having a cleaner structure with pyproject.toml that will fit all the needs 😄! It allows us to use tools such as pipx or just even pip install . inside the project and have a perfect ropgadget in our environment.

@n3rada
Copy link
Author

n3rada commented Oct 23, 2024

I didn't take the time to refactor all the code to follow all the best practices, I just wanted to give it a quick polish. 🧼

Is the quick polish has been automatically generated?

Not at all, 1h30 for the main things + a run of black well-known formatter.

@n3rada
Copy link
Author

n3rada commented Oct 23, 2024

Yeah, could you, please, remove the poetry part?

Poetry makes it really easy to manage and update project dependencies. Instead of manually editing requirements.txt, Poetry automatically resolves and locks versions of all dependencies (in poetry.lock) and allow you to respect with ease Python project management. If you really do not like the tool, I think my whole PR is trashable, since it basically move things in order to respect a PEP 621 project standard using poetry.

@n3rada
Copy link
Author

n3rada commented Oct 23, 2024

Btw, merging such large changes can cause another xz-utils) I need to find time to review all of this.

Totally understand! I don't want to add extra work for you. If testing these changes becomes too complicated, feel free to close the request. My main intention was just to help lighten your load!

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