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

Support for overriding SHA1 with URL rewriting #244

Closed
wants to merge 4 commits into from

Conversation

ceresek
Copy link

@ceresek ceresek commented Sep 29, 2021

When overriding a resource URL with urlrewrites or MX_URLREWRITES, it is not possible to substitute a modified resource with a different SHA1 because the subsequent SHA1 check fails. This patch extends the URL rewriting functionality with the possibility to specify a new SHA1 value to be checked against. It is useful for example when running modified benchmarks from local JAR files.

Possible issue. The URL rewriting rules are associated with URL patterns, SHA1 is associated with resources, a resource can have multiple URL values specified. This is probably not a problem from usability perspective, but the code modifications are somewhat awkward because URL rewriting and SHA1 checks happen at slightly different levels.

Tested only with native image benchmarking commands, may need testing with other suites.

@graalvmbot
Copy link
Collaborator

Hello Petr Tuma, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address petr -(dot)- tuma -(at)- d3s -(dot)- mff -(dot)- cuni -(dot)- cz. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

@ceresek
Copy link
Author

ceresek commented Sep 30, 2021

Sorry, my bad. I've signed the OCA with petr.tuma@acm.org. Should I resubmit the PR with a new mail ?

@graalvmbot
Copy link
Collaborator

Hello Petr Tuma, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address petr -(dot)- tuma -(at)- acm -(dot)- org. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

@ceresek
Copy link
Author

ceresek commented Sep 30, 2021

I've submitted the OCA version 1.7.1 in November 2019 for petr.tuma@acm.org and it worked back then.

@dougxc
Copy link
Member

dougxc commented Sep 30, 2021

Hi Petr, thanks for the PR. Could you please also update the "URL rewriting" section in README.md.

@dougxc
Copy link
Member

dougxc commented Sep 30, 2021

Possible issue. The URL rewriting rules are associated with URL patterns, SHA1 is associated with resources, a resource can have multiple URL values specified. This is probably not a problem from usability perspective, but the code modifications are somewhat awkward because URL rewriting and SHA1 checks happen at slightly different levels.

What exactly is the problem? I assume one will just need to ensure there's a rewrite rule for each URL of a resource whose sha1 you want to modify?

@ceresek
Copy link
Author

ceresek commented Sep 30, 2021

Hi Petr, thanks for the PR. Could you please also update the "URL rewriting" section in README.md.

Hi Doug, the doc was already updated, let me know if more info is needed ?

@ceresek
Copy link
Author

ceresek commented Sep 30, 2021

What exactly is the problem? I assume one will just need to ensure there's a rewrite rule for each URL of a resource whose sha1 you want to modify?

Actually, specifying SHA1 with the first URL should be enough. Since the SHA1 check is in a different place than the URL processing loop, the SHA1 rewriting code does not "know" what URL will be used. But in essence yes, it is not a usability problem per se.

@dougxc
Copy link
Member

dougxc commented Sep 30, 2021

Hi Doug, the doc was already updated, let me know if more info is needed ?

Sorry, I missed that. I thought I was looking at code comments.
I'll merge this PR.

@graalvmbot
Copy link
Collaborator

Petr Tuma has signed the Oracle Contributor Agreement (based on email address petr -(dot)- tuma -(at)- acm -(dot)- org) so can contribute to this repository.

@dougxc
Copy link
Member

dougxc commented Oct 7, 2021

Resolved by f235b8e.

@dougxc dougxc closed this Oct 7, 2021
ceresek added a commit to ceresek/fork-graal-mx that referenced this pull request Dec 8, 2021
ceresek pushed a commit to ceresek/fork-graal-mx that referenced this pull request Dec 9, 2021
ceresek pushed a commit to ceresek/fork-graal-mx that referenced this pull request Dec 16, 2021
ceresek pushed a commit to ceresek/fork-graal-mx that referenced this pull request Dec 16, 2021
ceresek pushed a commit to ceresek/fork-graal-mx that referenced this pull request Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants