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

update pahole and elfutils #899

Merged
merged 7 commits into from
Dec 16, 2022
Merged

update pahole and elfutils #899

merged 7 commits into from
Dec 16, 2022

Conversation

madhur4127
Copy link
Contributor

@madhur4127
Copy link
Contributor Author

Alternatively, I could make it point to trunk (master) to avoid updating this often

@partouf
Copy link
Contributor

partouf commented Dec 15, 2022

Thank you for doing this, I kept forgetting about this.

I'd rather have it not pointing to master though.

(there are some changes due that will make this easier than it is now anyway, so it wouldn't be much of a bother to update as it is now)

@madhur4127
Copy link
Contributor Author

Oops, I made it point to master, we can revert the topmost commit to undo.

fi

if [[ "$TARGET_PAHOLE_VERSION" != "$CURRENT_PAHOLE_VERSION" ]]; then
if [[ ! -f "${OPT}/pahole/bin/pahole" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

so this will only ever install once, right? Is the intention to install "trunk" pahole? (like the other PR implies?)

Copy link
Member

Choose a reason for hiding this comment

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

And by that I mean:

  • elsewhere trunk not only means trunk, but the current (at least built daily, trunk)
  • so I think we should mean the same here :)

This is OK as is but we should consider moving this to our yaml stuff wihch can deal with this much better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention is to point to master and build pahole everytime and install it in ${OPT}/pahole (this dir is unchanged). Instead of having to upgrade versions every now and then.

Digging more made me realize that this script is invoked by a cronjob (every 3h). So I think the best way is to download and build the pahole and then install the binaries in the end to overwrite existing ones.

Copy link
Member

Choose a reason for hiding this comment

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

This script is invoked daily at 3am, which is fin....we just need to remove the if check and it will install nightly.

In a separate PR we can make it build using our existing system. I'll make the change and merge in.

update_compilers/install_binaries.sh Outdated Show resolved Hide resolved
update_compilers/install_binaries.sh Outdated Show resolved Hide resolved
@madhur4127
Copy link
Contributor Author

new changes will always download and build pahole from the master branch

Copy link
Member

@mattgodbolt mattgodbolt left a comment

Choose a reason for hiding this comment

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

Ah! You made the change already, perfect. Thanks!

@mattgodbolt
Copy link
Member

I'll merge and fix the test errors

@mattgodbolt mattgodbolt merged commit 8b74dfc into compiler-explorer:main Dec 16, 2022
@madhur4127 madhur4127 deleted the madhur/update-pahole-elfutils branch December 16, 2022 13:21
mattgodbolt added a commit that referenced this pull request Dec 16, 2022
mattgodbolt pushed a commit that referenced this pull request Dec 19, 2022
* update pahole and elfutils nightly
mattgodbolt added a commit that referenced this pull request Dec 19, 2022
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.

[BUG]: Pahole corrupted output
3 participants