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

[bt#19975] Update pdfjs #17409

Open
wants to merge 1 commit into
base: 13.0.project_PR_449
Choose a base branch
from

Conversation

BT-oreinecke
Copy link

@BT-oreinecke BT-oreinecke commented Sep 7, 2022

git checkout d3bb55a ./addons/web/static/lib/pdfjs

I suppose pdfjs isn't update very often on the project branch. In this case, it showed a "bad XRef entry" instead of the PDF. Evince on the other hand was perfectly capable of rendering it.

Not sure if this is the way to go. I suppose it must be tested carefully and the component is very isolated. Let's see.

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

Links to Odoo:

git checkout d3bb55a ./addons/web/static/lib/pdfjs

I suppose pdfjs isn't update very often on the project branch. In this case, it
showed a "bad XRef entry" instead of the PDF. Evince on the other hand was
perfectly capable of rendering it.

Not sure if this is the way to go. I suppose it must be tested carefully and
the component is very isolated. Let's see.
@BT-oreinecke
Copy link
Author

Hi @BT-tbaechle

Not sure if this is the way to go. I've managed to see the pdfjs error in my local odoo as well (happens with any bad PDF, so it's not directly related to the PDF merge). Evince renders the PDF just fine.

Then I've tried updating the entire dir addons/web/static/lib/pdfjs via git checkout origin/14.0 ./ and it was fixed as if by magic.
So if you have no objections, I'll merge the PR and then create another one for the deploy yaml.

@BT-oreinecke
Copy link
Author

Hi @BT-pgaiser

Even though it seems to work just fine and it fixes the error messages in the merged PDFs, I'd like a second opinion about this.

You don't need to bother with the actual changes, all I did was cd into addons/web/static/lib/pdfjs and then git checkout origin/14.0 ./ and then committed everything.

@BT-pgaiser
Copy link

@BT-oreinecke
Well, the way it works with the odoo/enterprise code is that we don't really update it unless we have to or there are very important security patches. This update policy is super bad in my eyes, but unfortunately Odoo makes it quite hard for us because they don't mind breaking things left and right even within the same major version of their code. The amount of changed code is quite large here. If possible, perhaps it would be more practical to cherry pick only the affected part from lib/pdfjs. But the change has to be inspected either way for breaking changes (e.g. removed/renamed functions etc.).

I'm not entirely sure what the best approach here is.
@BT-tbaechle I'd like to get your opinion on this matter.

@BT-oreinecke
Copy link
Author

@BT-pgaiser I am absolutely open to a more minima changeset.

@BT-tbaechle
Copy link

@BT-oreinecke was pdfjs in the 13.0.project_PR_449 on the same commit/version as in 13.0? If not, I would try to update to the latest version on 13.0 before using pdfjs from 14.0. It's hard to tell if this could have any negative side effects. Have you checked if there is a specific version of pdfjs required/supported by Odoo13? As Phil pointed out, when it comes to the actual Odoo code, we only cherry-pick the relevant changes to minimize the impact, but I don't know if that is possible here. If you don't see a good way to limit the changes to just the relevant subset, and you think this won't have too big of an impact, then I would say go ahead and test parts of the system that could be impacted. Or you could try getting in touch with Odoo to report the issue, if you think there is nothing wrong with the PDF and Odoo should be able to handle it. Version 13.0 is not EOL just yet...
FYI @BT-pgaiser

@BT-oreinecke
Copy link
Author

Hi @BT-tbaechle

If not, I would try to update to the latest version on 13.0 before using pdfjs from 14.0.

Sorry I didn't mention it but I tried exactly that before using the 14.0's pdfjs. This would be my favorite solution as well but it didn't fix the issue unfortunately.

I'll check if I could cherry-pick just the relevant commits then.

@BT-oreinecke
Copy link
Author

(add no_merge because this isn't done yet, the other related PR is fine though)

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.

4 participants