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

Updated mvar.t to stream.t wherever needed #7

Closed
wants to merge 1 commit into from
Closed

Updated mvar.t to stream.t wherever needed #7

wants to merge 1 commit into from

Conversation

prernadabi23
Copy link

Updated the changes required. I tried to solve it, if there's any fault I will try to solve it again.
fixes: #4
Signed-off-by: Prerna Dabi [email protected]
@rand00

@rand00
Copy link
Owner

rand00 commented Mar 10, 2023

Thank you for working on this. I can see from the code changes that this won't compile - but this is a good start.

If you follow the instructions on the README.md to compile the unikernel (e.g. as a unix target to keep it simple), then you will see informative compiler-errors for what is expected/doesn't work.

First, try to see if you can make the compiler happy - and if you have any questions, just ask here (:

@0xrotense
Copy link

can I work on this :)?

@rand00
Copy link
Owner

rand00 commented Mar 13, 2023

can I work on this :)?

@0x0god - @prernadabi23 is already working on this, and I believe I saw you being active on another PR in another repository. Maybe finish one PR at a time

@rand00
Copy link
Owner

rand00 commented Mar 13, 2023

@prernadabi23 - have you got progress on compilation, or do you have questions on this?

@prernadabi23
Copy link
Author

@rand00 I am working on it. My laptop is having some issues, when I am done with that I'll finish this.

@prernadabi23
Copy link
Author

Screenshot from 2023-03-15 12-23-32

@rand00 I am having issues in running ocaml-platform. I was following this: "mirage/mirage#1402".

@rand00
Copy link
Owner

rand00 commented Mar 15, 2023

Hmm, I havn't used the ocaml-platform (I've set up my system manually), so don't know what can go wrong with it. Can you try to run opam init yourself and see what happens?

If that succeeds, try to run ocaml-platform again


Edit: I intentionally didn't give opam init the extra arguments, just run it as is

@rand00
Copy link
Owner

rand00 commented Mar 15, 2023

Ah just had a look at opam init --help - the errorcode 40 means:

Sync error. Could not fetch some remotes from the network. This can be a partial error.

Did you have internet access at the point of writing the command?

@rand00
Copy link
Owner

rand00 commented Mar 15, 2023

If it's the internet connection that was the problem, just try out running ocaml-platform without opam init first

@prernadabi23
Copy link
Author

okay I'll try it again.

@prernadabi23
Copy link
Author

Screenshot from 2023-03-18 20-06-15

Hello @rand00, I have done those steps successfully. What to do next?😁

@0xrotense
Copy link

is this issue fixed now?

@prernadabi23
Copy link
Author

prernadabi23 commented Mar 21, 2023

I am working on it @0x0god. I just have to complete the setup.

@0x0god Have you completed the setup? What are the steps to follow after the ss that I have attached above?

@prernadabi23
Copy link
Author

Hello @rand00, I am a little stuck and would appreciate your help.

@rand00
Copy link
Owner

rand00 commented Mar 22, 2023

Hello @rand00, I am a little stuck and would appreciate your help.

Sorry for the downtime - was sick since the weekend :/

Next step is to compile conntest - you can follow the instructions in the README: https://github.com/rand00/conntest#compiling

@prernadabi23
Copy link
Author

prernadabi23 commented Mar 23, 2023

Screenshot from 2023-03-23 18-42-02

I did the commands but I think there is some issue. So I checked this https://github.com/rand00/conntest/issues/1. But still, nothing changed.

@rand00
Copy link
Owner

rand00 commented Mar 23, 2023

Done! Now, How do I check the compiler errors that you were talking about?

So you earlier transformed the sourcecode a bit by replacing Lwt_mvar with Lwt_stream - this wouldn't compile. So try to compile contest with these changes and see how far you can get with making the compiler happy (:

@rand00
Copy link
Owner

rand00 commented Mar 23, 2023

I did the commands but I think there is some issue. So I checked this https://github.com/rand00/conntest/issues/1. But still, nothing changed.

So the last line before you cloned the conntest repository said that you should run eval $(opam env). This updates the terminal environment to see the current opam switch - otherwise the installed libraries etc. won't be visible

@prernadabi23
Copy link
Author

Screenshot from 2023-03-23 18-55-17
Screenshot from 2023-03-23 18-56-17

Is this correct?

@rand00
Copy link
Owner

rand00 commented Mar 23, 2023

It should now have generated a mirage/dist/conntest.spt - if it's there it worked (: Maybe run the whole command again replacing -t spt with -t unix - then you will get a mirage/dist/conntest which you can run directly in the terminal.

When you then begin to develop on conntest you can just run mirage build -f mirage/config.ml to recompile (which is pretty fast relative to reconfiguring the unikernel + compiling)

@prernadabi23
Copy link
Author

Yes, I am getting errors now.
@rand00

@prernadabi23
Copy link
Author

Is there any guide or reference that I can use?

@rand00
Copy link
Owner

rand00 commented Mar 24, 2023

There are several - what are the specific things you want to know about?

@rand00
Copy link
Owner

rand00 commented Mar 24, 2023

For OCaml here are some good references:

@rand00
Copy link
Owner

rand00 commented Mar 24, 2023

For finding out what is possible with the code at hand you can do several things:

  • Use "go to definition" on a module like Lwt_stream:
    Then you should end up in the mli file of that module, where you can see documentation for the different functions available. This file is for example present in ls $(opam var lib)/lwt/lwt_stream.mli- I often traverse the lib directory to find the modules and documentation I'm interested in.

  • Use autocomplete after writing Lwt_stream, then you can autocomplete on the available functions and see their types.

  • More modern methods of generating HTML documentation is also possible. E.g. using odig. There also exist some website online with documentation of packages, but can't remember where.

@prernadabi23
Copy link
Author

Thank you @rand00.

@rand00
Copy link
Owner

rand00 commented Mar 27, 2023

Remember that there is a deadline april 3. for making the contribution

@prernadabi23
Copy link
Author

@rand00 I was getting syntax errors, so I decided to learn more about the language.
I am trying!

@prernadabi23
Copy link
Author

prernadabi23 commented Mar 27, 2023

@rand00 I need help, I am just getting endless errors😭.

@rand00
Copy link
Owner

rand00 commented Mar 27, 2023

I'm sorry you have trouble with it - but it's normal that there is a high learningcurve if you havn't worked with typed functional languages before!

Can you explain what you find hardest?

@prernadabi23
Copy link
Author

I changed the file which needed change -
Lwt.async (fun () -> Lwt_mvar.put flow.sink ring_field);
to
Lwt_stream.push flow.sink ring_field;

then the compiler gave an error
then I did some changes again
then the error is
(snd flow.source)#push data >|= fun () -> ^^^^
Error: This expression has type ([> Data of Cstruct.t ], [> Msg of string ]) result but an expression was expected of type (Cstruct.t Mirage_flow.or_eof, err) Lwt_result.t = (Cstruct.t Mirage_flow.or_eof, err) result Lwt.t

and I now don't know how to solve it.
@rand00

@rand00
Copy link
Owner

rand00 commented Mar 28, 2023

It's smart to start making changes from "one end" of the code, and then make the rest of the code "conform" to that change. So in this case, the type of flow.sink needs to be the right one. With Lwt_stream.t there is both a bounded version and an unbounded version - two type aliases for these are here: https://github.com/rand00/conntest/blob/main/lib/conntest.ml#L163-L164

The difference between these two types is how to push new elements to the stream. Let's take outset in the 'a stream because I think we want an unbounded stream. The ('a option -> unit) part of this type is the push-function for the stream. To use it, one would then call (snd flow.sink) some_new_element.

It's hard to help with the specific error when I don't have the code, but concerning the category of error you are getting, it's about what type of monad you are in. So in MirageOS, the Lwt_result monad is used a lot, which is a stack of two monads: the Result monad and the Lwt monad. You actually see that concrete type in the last error:

... but an expression was expected of type (Cstruct.t Mirage_flow.or_eof, err) Lwt_result.t = (Cstruct.t Mirage_flow.or_eof, err) result Lwt.t

In the error it's complaining that the last value in the expression is missing the Lwt.t part of the type. This can be done in several ways, either by using another Lwt operator (map vs bind) or by wrapping the last returned value in Lwt.return.

I can also understand that this part of the error is confusing: This expression has type ([> Data of Cstruct.t ], [> Msg of string ]) result - as this hasn't got the same name as the later mentioned (Cstruct.t Mirage_flow.or_eof, err) result. What is happening here is that the first type is a subtype of the second - in OCaml there is structural subtyping for polymorphic variants and for objects.

If you go to definition the type Mirage_flow.or_eof on line https://github.com/rand00/conntest/blob/main/lib/conntest.ml#L185 - then you will se that it's a polymorphic variant:

type 'a or_eof = [`Data of 'a | `Eof ]

@rand00
Copy link
Owner

rand00 commented Mar 28, 2023

Btw. in the top conntest.ml some operators are included in the scope by opening some modules: https://github.com/rand00/conntest/blob/main/lib/conntest.ml#L1-L2

It's possible to go to definition on these.

Lwt.Infix includes e.g. the operators >>= which is Lwt.bind and >|= which is Lwt.map.

Lwt_result.Syntax e.g. introduces let* which is Lwt_result.bind, and let+ which is Lwt_result.map.

The reason why both Infix and Syntax is used is just for convenience of always having operators available for both the Lwt and Lwt_result monads. Another more clean solution would have been to locally open the Syntax modules or call the functions directly when needed - e.g. call Lwt.map instead of >|=.

@prernadabi23
Copy link
Author

@rand00 I am new to Ocaml, It would take time to learn it and to implement it in the code which is basically a hard task.
I was reading and changing the code but still it will take time. I am sorry for the delay.

@rand00
Copy link
Owner

rand00 commented Mar 28, 2023

It is totally understandable that it's hard when you are new. It is really cool that you've been motivated and gotten further towards a PR! Hope you can keep up the motivation, and remember to ask for advice if you get stuck.

@prernadabi23
Copy link
Author

In the final application, what should I write in the timeline?
@rand00

@rand00
Copy link
Owner

rand00 commented Mar 30, 2023

You should just write what you think makes sense - the timeline is not "written in stone", but a suggestion for what will happen. You can look at the existing project description and write down the issues you would like to work on during the internship.

Remember that you need to have comitted something to a PR to be accepted - the PR doesn't have to be finished by the deadline, but there should be some progress.

@prernadabi23
Copy link
Author

Is there anything else an extension or package that we need to have on vscode?
@rand00

@rand00
Copy link
Owner

rand00 commented Mar 31, 2023

What is the issue with VSCode? Do you e.g. have type-checking and autocompletion?

@rand00
Copy link
Owner

rand00 commented Mar 31, 2023

I havn't tried VSCode, but I can see that there is some extension that seems to be needed:
https://marketplace.visualstudio.com/items?itemName=ocamllabs.ocaml-platform

I found that link on this guide for setting up your editor: https://ocaml.org/docs/up-and-running#configuring-your-editor

@prernadabi23
Copy link
Author

https://docs.google.com/document/d/1Ja2gOdcFU5n03FvDP2ZScqSI1lZd1cngk5ObH8BN2UA/edit?usp=sharing

I had made a lot of changes, but I undid them. Now, I am wondering if this was the right way to solve the issue. Therefore, I created a file detailing the changes I have made since the beginning.

I was thinking that there may be some packages that are not installed, as I am getting some errors that I cannot find a solution for.

@prernadabi23
Copy link
Author

I havn't tried VSCode,

Which platform do you recommend then?

@rand00
Copy link
Owner

rand00 commented Mar 31, 2023

Which platform do you recommend then?

I think VSCode is fine from what I hear, so I recommend that, as it's a simpler editor than e.g. emacs or vim. Personally I use emacs.

Thanks for the document - is well structured - but would make more sense to push the code-changes to this PR instead, as I would be able to comment directly on the code here via Github then (and run the code easily myself). This will also count more towards the internship application (code doesn't need to compile, even though it's preferred).

I'll try to comment here based on the document:

  • Line 510:
    • Error: "Error: Unbound value Lwt_stream.push"
      • The interface of Lwt_mvar and Lwt_stream are not the same, even though they have related semantics. I earlier mentioned that when you call Lwt_stream.create you get a stream and its push-function in a tuple. That is why I've made a type-alias for this at line 164. This is also related to a later problem you have.
  • Added open Lwt_stream:
    • In OCaml it's bad style to open a module, as you then get all its values into scope, which can override your own values (or in this case override standard library types) - and the reader of the code don't know where the values come from, when the open statement is further upwards in the text-file.
      • This is why you get some of your errors, because some type result from the Lwt_stream module overrides the standard result type
  • Changed the source field of the partial_flow type:
    • It should not be the source field, but the sink field that is changed from an Lwt_mvar to an Lwt_stream
      • hint: use the 'a stream type alias that I mentioned earlier
  • The error on line 172 is also because the earlier open Lwt_stream

Try to fix these things and it should become less confusing.

Another thing: it's both the sink field of the partial_flow and the t types that need to be changed. (In another issue these two types will later be merged)

@prernadabi23
Copy link
Author

The extension ocamllsp is quite useful, I get to know about the error even without compiling it :)

@prernadabi23 prernadabi23 closed this by deleting the head repository Mar 31, 2023
@rand00
Copy link
Owner

rand00 commented Mar 31, 2023

Good that you got it working \o/. Was it an error that you closed the PR?

@prernadabi23
Copy link
Author

Yes I don't know why I was not able to commit in this branch.

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.

Instead of using Lwt.async + Lwt_mvar when receiving UDP datagram, use an Lwt_stream.t
3 participants