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

Small fixes of local build of gprofiler executable #941

Closed
wants to merge 10 commits into from

Conversation

dkorlovs
Copy link

@dkorlovs dkorlovs commented Mar 7, 2025

Added small changes to fix issues that we faced with build locally. It will be useful to have these changes in branch also, for easier development.

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots

Checklist:

  • I have read the CONTRIBUTING document.
  • I have updated the relevant documentation.
  • I have added tests for new logic.

@dkorlovs dkorlovs requested a review from mlim19 March 7, 2025 12:33
@awherr
Copy link
Contributor

awherr commented Mar 11, 2025

Do you think we can merge this now?

@dkorlovs
Copy link
Author

Do you think we can merge this now?

We need to discuss changes with "--trusted-host files.pythonhosted.org --trusted-host pypi.org" in docker file with @mlim19.
But perfspect collection part is ready, and python linter issues fixed.

@mlim19
Copy link

mlim19 commented Mar 11, 2025

This patchset combines 2 things: Ron's patch for workaround the proxy issue and our patch for initial perfspect integration which I think not mature yet to merge. And they should be split and merged separately when ready.

Regarding Ron's patch, I believe the pip config change seems more suitable for proxy users which is described at https://stackoverflow.com/questions/25981703/pip-install-fails-with-connection-error-ssl-certificate-verify-failed-certi
The pip config change seems better because we can delegate the workaround adoption to users instead of forcing it to every user. Users don't take the workaround if not needed. I will verify it and update later.

@awherr
Copy link
Contributor

awherr commented Mar 11, 2025

I agree that it makes sense to split these up, with the proxy workaround going first. The reasons for asking is that I want to close out the remaining CVEs without disrupting the process. I will work from this branch for now.

@dkorlovs
Copy link
Author

This patchset combines 2 things: Ron's patch for workaround the proxy issue and our patch for initial perfspect integration which I think not mature yet to merge. And they should be split and merged separately when ready.

Regarding Ron's patch, I believe the pip config change seems more suitable for proxy users which is described at https://stackoverflow.com/questions/25981703/pip-install-fails-with-connection-error-ssl-certificate-verify-failed-certi The pip config change seems better because we can delegate the workaround adoption to users instead of forcing it to every user. Users don't take the workaround if not needed. I will verify it and update later.

Let me split changes, I will prepare PR to enable_perfspec_metrics branch with changes related only to perfspect integration.
And after that I'll close this PR.

@dkorlovs
Copy link
Author

This patchset combines 2 things: Ron's patch for workaround the proxy issue and our patch for initial perfspect integration which I think not mature yet to merge. And they should be split and merged separately when ready.
Regarding Ron's patch, I believe the pip config change seems more suitable for proxy users which is described at https://stackoverflow.com/questions/25981703/pip-install-fails-with-connection-error-ssl-certificate-verify-failed-certi The pip config change seems better because we can delegate the workaround adoption to users instead of forcing it to every user. Users don't take the workaround if not needed. I will verify it and update later.

Let me split changes, I will prepare PR to enable_perfspec_metrics branch with changes related only to perfspect integration. And after that I'll close this PR.

PR with changes related only to Perfspect integration:
#942

@dkorlovs dkorlovs closed this Mar 11, 2025
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