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

Refactor crate structure #4

Closed
maxammann opened this issue Apr 18, 2022 · 8 comments · Fixed by #12
Closed

Refactor crate structure #4

maxammann opened this issue Apr 18, 2022 · 8 comments · Fixed by #12

Comments

@maxammann
Copy link
Collaborator

maxammann commented Apr 18, 2022

.
├── .cargo
├── .github
├── docs
├── maplibre
├── maplibre-android
├── maplibre-apple
├── maplibre-benches
├── maplibre-renderer
├── maplibre-example
├── maplibre-web
├── maplibre-style-spec
├── maplibre-tilejson-spec
├── maplibre-wgsl-validate
├── justfile
├── Cargo.toml
├── LICENSE
├── README.md
├── rust-toolchain.toml
├── .gitignore
@nyurik
Copy link
Member

nyurik commented Apr 21, 2022

I just saw there was an issue about it (thx!). I think maplibre-* prefix makes it a bit too busy and cumbersome to use -- everyone will keep having to type cd map<TAB>-ap<TAB> instead of cd ap<TAB>, plus a lot of extra letters everywhere... How about hte same but without the prefix?

@maxammann
Copy link
Collaborator Author

Let's discuss this here :) So I don't really have a personal preference here because I'm quite new to Rust workspaces. I tried to follow the best/most common practice here:

Some other keep "sub-crates" in a crates directory which just adds unnecessary hierarchy: https://github.com/rust-lang/cargo/tree/master/crates

The rule is: "Put subcrates at the top-level and name the directories like the name of the crate."

@nyurik
Copy link
Member

nyurik commented Apr 21, 2022

Yep, makes sense to me. I would just add that i think most creates would not need to be named maplibre-foo -- if it's a good piece of functionality, it's probably not related to maplibre (e.g. tilejson parser). And if it is usable only by maplibre, we probably won't publish it by itself. I'm sure there are exceptions to this, but we can always just use the prefixed name in the crates.toml, while keeping the dir structure simple

@maxammann
Copy link
Collaborator Author

true, but I think it makes sense to use a generic name like style-spec or tilejson as soon as we want to release it. as a separate create. The thing is that we will need to release all the creates to creates.io. I think we should not "burn" these names right now.

So the creates have to stick with maplibre-style and maplibre-tilejson for now. The question is whether we want to break consistency by having a the directories style/ and tilejson/.

@nyurik
Copy link
Member

nyurik commented Apr 21, 2022

Below is my "philosophy", which could be totally wrong, please don't take it as gospel :)

Ok, so there are two separate concerns we are trying to address: code modularity and code reusability. Packages (crates) are only needed for reusability -- so that other projects can use some functionality directly, without the need to use the whole project. We should only publish crates that we feel fit that description - smallish self-contained task-oriented reusable pieces of code. On the other hand, we can achieve modularity with a dir tree structure inside a single crate, e.g. /src/<foo>/<bar>/..., esp when we are not certain which code would be usable on its own.

updated proposal: keep everything in a single crate /maplibre/src/... with submodules, until a specific functionality emerges that we want to share with others. When it does, move it to the root dir as /some-functionality/src/..., publish, and link to it from /maplibre/Crates.toml - [dependencies].

@maxammann
Copy link
Collaborator Author

After sleeping over this and taking in your "philosophy" I found a new solution. I think you are right, except for one point: Crates are not only used for re-usability but also for "packaging" (packing as binary, shared lib, static lib). This is important for the different library targets which are needed for android and iOS.

The styles and tilejson should be modules, you are right.

So here is my new proposal:

├── .cargo
├── .github
├── docs
├── maplibre
            ├── styles
            ├── tilejson
├── maplibre-example
            ├── Cargo.toml
├── maplibre-build-tools
            ├── Cargo.toml
├── android
            ├── gradle/
            ├── Cargo.toml
├── apple
            ├── xcode/
            ├── MapLibreSwiftPackage
            ├── Cargo.toml
├── web
            ├── package
            ├── Cargo.toml
├── benches
├── justfile
├── Cargo.toml
├── LICENSE
├── README.md
├── rust-toolchain.toml
├── .gitignore

maplibre-build-tools needs to be published because it contains code for building the maplibre crate. maplibre-example should also be published, as a demo application. Maybe we should name it ``maplibre-demo. It will be installable as binary through cargo install maplibre-example`.

Here are some rules:

  • Creates which start with maplibre will be published
  • Creates which are used to build static/shared libraries for android and apple are not published

Some more thoughts:

  • We could think about putting apple, android and web into a platforms directory. All of these crates will contain some platform specific code. For example JNI headers for Android, headers for Apple.

@maxammann
Copy link
Collaborator Author

If we decide at some point that functionality should be published, then we will extract code from maplibre, apple, android, web.

@nyurik
Copy link
Member

nyurik commented Apr 22, 2022

This sounds much better, thanks!!!

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 a pull request may close this issue.

2 participants