Skip to content

Fix issue with bytes_to_string in library. #5580

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

Merged
merged 2 commits into from
Jul 22, 2022
Merged

Conversation

cristianoc
Copy link
Collaborator

Fixes #5573

@cristianoc
Copy link
Collaborator Author

@TheSpyder take a look at this fix -- does it work for you?

@cristianoc
Copy link
Collaborator Author

cristianoc commented Jul 22, 2022

The situation of bytes_of_string seems even worse in that it looks like it's not defined anywhere. Nor is seems to be used anywhere. Probably better to just remove it.

@cristianoc cristianoc merged commit cf177c6 into master Jul 22, 2022
@cristianoc cristianoc deleted the bytes_to_string branch July 22, 2022 17:25
@TheSpyder
Copy link
Contributor

That looks like it'll work, yes. The Char library file no longer references a method that doesn't exist :)

@TheSpyder
Copy link
Contributor

I wasn't paying enough attention. RC1 has the same issue - the code I said replaced Caml_bytes isn't Bytes.bytes_to_string, it's Bytes.unsafe_to_string.
https://github.com/rescript-lang/rescript-compiler/blob/375cc68473d08d8357531b7cb48a91fec27a34f5/jscomp/stdlib-406/bytes.ml#L158

@cristianoc
Copy link
Collaborator Author

Would you create a PR?

@TheSpyder
Copy link
Contributor

I'm trying, but figuring out how to run a locally built compiler in my project is turning into a hassle. I got it working, but after changing the reference and rebuilding it was completely broken 🤷‍♂️ I guess I could still make the PR, the code changes are correct now.

@cristianoc
Copy link
Collaborator Author

cristianoc commented Jul 24, 2022

I'm trying, but figuring out how to run a locally built compiler in my project is turning into a hassle. I got it working, but after changing the reference and rebuilding it was completely broken 🤷‍♂️ I guess I could still make the PR, the code changes are correct now.

Thank you.
Did you figure out what the issue with building the compiler was? Any suggestions to improve the contributors guide?

(there's a makefile too though it's ongoing. For your case: "make clean && make test" should do.
Plus, installing the locally built compiler into a project)

@TheSpyder
Copy link
Contributor

Ah I was just following the contribution guide and didn’t notice the Makefile. Thanks for the tip, I’ll see if that helps later this afternoon.

@TheSpyder
Copy link
Contributor

Didn't help. I'm at a loss as to why sometimes the local build works in my project and other times it fails with a syntax error.

@cristianoc
Copy link
Collaborator Author

cristianoc commented Jul 24, 2022

If it can "help", the build is no deterministic. Still haven't figured out where but definitely there are race conditions.
To be confirmed if telling ninja "-j 1" solves it.

@TheSpyder
Copy link
Contributor

The build completes successfully, it's the compiler executable it builds that is behaving differently. It's acting like it has a different version of the syntax that isn't quite right.

@cristianoc
Copy link
Collaborator Author

Is the same executable behaving differently or is there a recompile in between?

@TheSpyder
Copy link
Contributor

A recompile. On two macs it worked once and then I switched to my branch and never again, even after switching back to master and cleaning everything.

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.

bytes_to_string is missing
2 participants