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

Move vendored shards to "src/compiler/vendor/" #13784

Closed
nobodywasishere opened this issue Sep 3, 2023 · 4 comments · Fixed by #13992
Closed

Move vendored shards to "src/compiler/vendor/" #13784

nobodywasishere opened this issue Sep 3, 2023 · 4 comments · Fixed by #13992

Comments

@nobodywasishere
Copy link
Contributor

Feature Request

This is related to #13040

On Arch Linux, the lib/ folder is not included in the distribution of the compiler, meaning parts of the compiler can't be re-used when writing programs (such as the parser, etc). The distribution could include this lib folder, but it would be in a very weird place. As an alternative to the current layout, I would like to propose moving the vendored shards to "src/compiler/vendor/", that way instead of requiring like:

require "../../../../../lib/markd/src/markd"

The compiler would instead require:

require "src/compiler/vendor/markd/src/markd"

And all the source code for the compiler would be packaged and integrated. This would make writing scripts that use the compiler itself easier. This shouldn't break backwards compatibility, and should avoid the issue previously ran into with overriding packages projects have specified in their shards.yml.

@straight-shoota
Copy link
Member

I symphasize with the idea to make using compiler code for third party tools easier.

This change is relatively simple. But I'm not convinced it's a good idea.

The purpose of the lib folder is a clearly separated place for external code, vs. owned code in src.
This has many small practical implications, for example regarding formatter style across the code base. Moving external code into src would make some tasks unnecessarily complex because you would always have to explicitly exclude the vendored directory.

I think it's better for this project to keep depencies in a separate lib folder. This conforms to the common layout of Crystal projects (shards puts dependencies into lib as well).

There are other options to improve the usability of compiler code for third party tools.
I'm thinking #13040 was actually a good change and we could consider going back to that. This would allow installing shard dependencies explicitly and the compiler will pick them up accordingly.

@nobodywasishere
Copy link
Contributor Author

I think that's a good idea. I have a few concerns about switching back to #13040:

  • Will markd/reply/sanitize conflict if someone adds those shards to their shard.yml, esp if they're different versions? (then again these requires only happen when someone's reusing the compiler parts so this shouldn't be an issue)
  • Where/how should OS packagers put the contents of lib/? For instance, arch linux puts the contents of src/ directly into /usr/lib/crystal/, not really leaving a place for the lib/ folder to go

@straight-shoota
Copy link
Member

  • There should be no conflicts. Local lib folder takes precedence. Actually, this would give more flexibility in choosing the specific dependency version.
  • They could merge lib with src. But I think they should not ship these dependencies at all. If you want to use them, you can install them explicitly as dependencies in your project via shards.

@nobodywasishere
Copy link
Contributor Author

That would work. I'd just be concerned about conflicts between the version crystal's known to work with and the version they set in their shards.yml, but that isn't that big of a problem.

Should I open a PR reverting the reverting commit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@straight-shoota @Blacksmoke16 @nobodywasishere and others