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

pre-commits generates pandoc-*.deb when converting rst to markdown #262

Closed
sbidoul opened this issue Jun 12, 2024 · 18 comments · Fixed by #265
Closed

pre-commits generates pandoc-*.deb when converting rst to markdown #262

sbidoul opened this issue Jun 12, 2024 · 18 comments · Fixed by #265
Labels
bug Something isn't working

Comments

@sbidoul
Copy link
Member

sbidoul commented Jun 12, 2024

See for example this commit: OCA/l10n-spain@6ab2763#commitcomment-143043458

Circumstances when this happens need to be clarified. It may come from the readme generator when it converts markdown back to rst, when pandoc is not yet installed on the machine.

@sbidoul sbidoul added the bug Something isn't working label Jun 12, 2024
sbidoul referenced this issue in OCA/l10n-spain Jun 12, 2024
@pedrobaeza
Copy link
Member

Someone can make a CI check like the dependencies one that goes to red when a .deb is present? Or give me any pointer on how to do it. We will need later a mass update on 17.0 branches.

Another thing is to remove completely these files, so the only solution is to forced push a clean branch without this. Do you agree?

@sbidoul
Copy link
Member Author

sbidoul commented Jun 14, 2024

Pre-commit has a way to fail on specific files: https://pre-commit.com/#fail
We coud add that to the template.

Another thing is to remove completely these files, so the only solution is to forced push a clean branch without this. Do you agree?

No, please, never ever force-push on main branches. That will cause all sorts of problems.

@pedrobaeza
Copy link
Member

We can't deal with 30 MB of diff adding it and another 30 MB of diff removing them, and if you dare to do git pull in any branch, you will have the same size increasing. That's a lot of overhead and provoking unneeded waste of resources.

The systems should fetch origin branch + reset --hard origin/branch for a secure auto-update. It's how we do it.

@carmenbianca
Copy link
Member

I agree that the cumulative waste of resources merits a force push, even if it's unfortunate and inconvenient.

@pedrobaeza
Copy link
Member

It's even worse now in some repos, like OCA/social, where there's 2 .deb with different versions: https://github.com/OCA/social/tree/17.0, so that 60 + 60 = 120 MBs

@pedrobaeza
Copy link
Member

@gurneyalex
Copy link
Member

Could we have a global *.deb line in the template gitignore as a mitigation measure?

@pedrobaeza
Copy link
Member

Yeah, that can be another option indeed...

@gurneyalex
Copy link
Member

gurneyalex commented Jun 27, 2024

#263

But not all repos will be synced in time so having this in the migration instruction is still useful.

@sbidoul
Copy link
Member Author

sbidoul commented Jun 27, 2024

Thanks @gurneyalex. Hopefully someone can spare the time to look at the root cause.

@sbidoul
Copy link
Member Author

sbidoul commented Jul 4, 2024

The fix has landed OCA/maintainer-tools#620. Thanks @nilshamerlinck, who also suggests the procedure to remove and force-push in OCA/maintainer-tools#619 (comment)

Now we need to update this template with the last maintainer tools commit. I'll then take care to push the template update, at least to 17.0 branches.

Regarding force push, this will make some commits on the affected main branches unreachable, so this could also cause troubles for folks who pin OCA commits directly.

@pedrobaeza
Copy link
Member

@sbidoul are you going to do the .deb cleaning?

@sbidoul
Copy link
Member Author

sbidoul commented Jul 4, 2024

I'm not convinced we should do it, but I'll not block. I do think this should communicated clearly and in advance, though, as this is disruptive to many workflows.

@pedrobaeza
Copy link
Member

OK, I will do it. I know it's not the best, but a lot of people do git pull freely in any local branch of any version (which fetchs everything from the origin remote), and wasting that amount of downloaded MBs is too much IMO.

@sbidoul
Copy link
Member Author

sbidoul commented Jul 6, 2024

The latest repo template is being pushed to 17.0 branches.

@pedrobaeza
Copy link
Member

I have found that there's also another package format: .pkg. Please include it merging #269 and releasing a new template. It seems this is also a problem in 16.0: OCA/delivery-carrier#864, so please deploy it in all the 16.0 branches.

@ap-wtioit
Copy link
Contributor

ap-wtioit commented Sep 24, 2024

@pedrobaeza here is a migration pull request that still had the .deb files OCA/server-auth#694, the rm step is no longer in the documentation for migration. Was this a user error or is it still happening for migrations to 17.0?

@pedrobaeza
Copy link
Member

I removed it because it was causing another side effect: the repository that already contained some .deb added the removal diff to the migration PR...

Theoretically, the fix was applied for not happening again. @sbidoul did you update all the repos templates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants