OSM-binary icon indicating copy to clipboard operation
OSM-binary copied to clipboard

Published 20 hours ago • verified publisher icon openstreetmap

Duplicate https://github.com/openstreetmap/osmosis/pull/109

Open simonpoole opened this issue 2 years ago • 2 comments

The PR https://github.com/openstreetmap/osmosis/pull/109 , once everything has been tested and merged, should be applied here too.

simonpoole avatar Mar 18 '22 16:03 simonpoole

@simonpoole Anything still to do here?

joto avatar May 27 '22 09:05 joto

Yes, this weekend (likely).

simonpoole avatar May 27 '22 10:05 simonpoole

@simonpoole ping?

joto avatar Oct 02 '23 08:10 joto

I was actually thinking on working on it when I get back next week.

simonpoole avatar Oct 02 '23 10:10 simonpoole

Hello @joto I am a new contributor I saw this open issue can you share the base branch, I will create a PR from "Fix_OSMHeader_block_compression" to the base branch

Also @simonpoole is there any channel I can join to be in sync with the issues and project updates?

divyanshuagarwal-23 avatar Oct 06 '23 15:10 divyanshuagarwal-23

I was actually thinking on working on it when I get back next week.

Hmm this wasn't actually the issue I was thinking about when I wrote that. Anyway @divyanshuagarwal-23 this wouldn't really seem to be a good beginner issue, but by all means feel free to to port the PR if it is even still necessary.

simonpoole avatar Oct 07 '23 16:10 simonpoole

@simonpoole I would like to work on the core codebase of OSM-binary, and need initial mentorship please help me with this

divyanshuagarwal-23 avatar Oct 12 '23 16:10 divyanshuagarwal-23

Do you have any slack channel that I joining and learn

divyanshuagarwal-23 avatar Oct 12 '23 16:10 divyanshuagarwal-23

If I understand this issue correctly it is about the OSMHeader file block that should not be compressed. But there is only one place where we create such a file block and that is in a test file where compression is set to null. So I don't see how we can apply this patch from Osmosis here!?

@simonpoole Did I misunderstand something here? Can we close this?

joto avatar Jan 06 '24 09:01 joto

Sigh, the confusion lies here https://github.com/openstreetmap/osmosis/pull/109#issuecomment-1080614565

When I saw @brettch comment I mistakenly assumed he had removed all vendored bits wrt pbf handling. But that only applies to osmosis-osm-binary and not osmosis-pbf (which is the bit that writes OSMHeader file blocks).

So the only change necessary -here- is the mini change to expose the CompressFlag enum so that it can be used in the osmosis package. See https://github.com/openstreetmap/osmosis/pull/109/files#diff-e98fe772c729b5020654d4563dcb3c03bdf132f91a351adcd3a39467262bb0cb

I'll create a PR today or tomorrow. Sorry for the confusion.

PS: it could be argued that this was always a bug as the public method BlockOutputStream.write(FileBlock block, CompressFlags compression) references the only package level visible CompressFlags.

simonpoole avatar Jan 07 '24 09:01 simonpoole