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

feat: include runtime from current directory instead of lib #2884

Merged
merged 3 commits into from
Nov 11, 2024

Conversation

sasq64
Copy link
Contributor

@sasq64 sasq64 commented Nov 8, 2024

Changes proposed in this pull request:

The runtime entry points (index-*.ts) all include their child modules from lib instead
the current directory.

This causes problems in particular when you try to base your custom runtime on the standard runtime, since it will ignore any code and just include from the standard library embedded in the compiler instead.

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@CountBleck CountBleck changed the title Include runtime from current directory instead of lib feat: include runtime from current directory instead of lib Nov 8, 2024
@CountBleck
Copy link
Member

Question: if you're basing your runtime off of the standard library, wouldn't you end up copying/vendoring those files into your project?

If so, wouldn't you be able to modify those imports manually?

@sasq64
Copy link
Contributor Author

sasq64 commented Nov 8, 2024

Question: if you're basing your runtime off of the standard library, wouldn't you end up copying/vendoring those files into your project?

If so, wouldn't you be able to modify those imports manually?

Yes, that's exactly what I did, but it took me quite some time to figure out why my changes had no effect.

@CountBleck
Copy link
Member

Okay, could you merge/rebase the latest changes pushed to main?

@HerrCai0907
Copy link
Member

I cannot reproduce this issue, could you explain more about it? or give a reproduce repo link. Thanks!

@sasq64
Copy link
Contributor Author

sasq64 commented Nov 11, 2024

I cannot reproduce this issue, could you explain more about it? or give a reproduce repo link. Thanks!

Copy node_modules/assemblyscript/std/assembly/rt to rt and try to use it with
--runtime rt/index-incremental.ts

First you notice that you need to fix imports to parent directory (../util and ../shared) but then it seems to work.

However, imports like

import "rt/tlsf";
import "rt/itcms";

actually import from the runtime built into the compiler which makes it ignore any changes you make in your local copy, which is very confusing.

And even if you don't use a custom runtime, I think you should always import relative files with ./ etc and not rely on the lib path?

@HerrCai0907
Copy link
Member

ahhh, it is real confused to mix library and compiler builtin library. it needs more test cases if we want to use this things in wider scenario.

Copy link
Member

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. thanks!

@CountBleck
Copy link
Member

ahhh, it is real confused to mix library and compiler builtin library. it needs more test cases if we want to use this things in wider scenario.

Yeah, it looks weird to an outside reader, and it's probably not good style either...

@CountBleck CountBleck merged commit 285afb1 into AssemblyScript:main Nov 11, 2024
14 checks passed
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 this pull request may close these issues.

3 participants