Add new backend to Move compiler targeting Solana VM

[dmakarov]

These changes implement Move to Solana VM compiler as a new backend of the the existing Move compiler with move-cli controlling the compilation and invoking the solana backend on modules of a Move package via proper function calls.

The move-mv-llvm-compiler cli remains unchanged. The compiler itself is moved to languages/solana subdir.

[ksolana]

This looks much more scalable approach. i'll fait for PR to be ready for the review but the intent of this change LGTM.

[jcivlin]

I can approve it but I want to understand better if it is true that you replaced all previous cli testing with one language/solana/move-to-solana/tests/rbpf-tests/basic test? Or if there are more here please point me to them - with this number of changes it may be just difficult to spot. (I understand that there is now all bunch of the original Diem move-cli tests, but it is not the same. We had some facility, I want to see that this has not gone).

I think we need tests that use -p option directly, likewise --test, --dev. And for the -p it may be useful to have the tests for relative, absolute and bad paths. I believe they all work as in the move-cli package, but we need to test it.

It is Ok if we have these tests later, but I want to know what is the plan.

[dmakarov]

I can approve it but I want to understand better if it is true that you replaced all previous cli testing with one language/solana/move-to-solana/tests/rbpf-tests/basic test? Or if there are more here please point me to them - with this number of changes it may be just difficult to spot. (I understand that there is now all bunch of the original Diem move-cli tests, but it is not the same. We had some facility, I want to see that this has not gone).

I think we need tests that use -p option directly, likewise --test, --dev. And for the -p it may be useful to have the tests for relative, absolute and bad paths. I believe they all work as in the move-cli package, but we need to test it.

It is Ok if we have these tests later, but I want to know what is the plan.

Yes, I removed --stdlib, -p, --test, and --dev options. The --stdlib option is obsolete by using move-cli and Move.toml to find the standard library sources. The -p flag is handled by move-cli. I made these changes so that the move-to-solana application would not replicate the functionality provided by move-cli, and -p flag was one instance of such replicated functionality. The --test option is used to create a test plan for move-compiler, and I don't see why we need this in a separate tool move-to-solana. The -dev option was passed to move-mv-llvm-compiler in stdlib-tests and stdlib-with-option-p-tests but I didn't find any use of it. So it's gone too. I removed stdlib-tests, stdlib-with-option-p-tests, and cli-tests as irrelevant if the changes in this PR are merged.

For using -p with absolute paths I currently do not have a test. The test harness is not flexible to test this both with relative and absolute paths, other than adding another top-level test for just running move-cli -p with absolute path to a package. This would test move-cli but not the move-to-solana compiler. Currently, test harness invokes move-cli with relative path argument to -p option. I'll think how to test move-cli with absolute paths given to the -p option.

[dmakarov]

I'll add more tests that compile Move packages in test and dev modes.

[jcivlin]

The -p flag is handled by move-cli.

Yes, it is assumed. But it has to be proven by testing. I see a test with dependency set in Move.toml, but we need an integration tests with -p option. And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

[dmakarov]

The -p flag is handled by move-cli.

Yes, it is assumed. But it has to be proven by testing. I see a test with dependency set in Move.toml, but we need an integration tests with -p option. And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

https://github.com/solana-labs/move/pull/304/files#r1307565893

[dmakarov]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

[jcivlin]

The -p flag is handled by move-cli.

Yes, it is assumed. But it has to be proven by testing. I see a test with dependency set in Move.toml, but we need an integration tests with -p option. And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

https://github.com/solana-labs/move/pull/304/files#r1307565893

It is in the code, yes. And I'm talking about the integration tests. In the removed cli-tests we tested the absolute path. We need to know that this works and if there is an error in the path it is handled.

[jcivlin]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

[dmakarov]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

I can quote my earlier comment

For using -p with absolute paths I currently do not have a test. The test harness is not flexible to test this both with relative and absolute paths, other than adding another top-level test for just running move-cli -p with absolute path to a package. This would test move-cli but not the move-to-solana compiler. Currently, test harness invokes move-cli with relative path argument to -p option. I'll think how to test move-cli with absolute paths given to the -p option.

I'm thinking about adding more directives to test harness, for example via comments in Move.toml file, for specifying absolute or incorrect paths to Move packages. In my view this is inappropriate to have such facility in tests for move-to-solana package, as this package is not related to move-cli. Nevertheless, I'll add it for the sake of better coverage. There are many other components in move repository that we do not directly test, and I don't think it's relevant to this PR.

[jcivlin]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

[jcivlin]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

I can quote my earlier comment

For using -p with absolute paths I currently do not have a test. The test harness is not flexible to test this both with relative and absolute paths, other than adding another top-level test for just running move-cli -p with absolute path to a package. This would test move-cli but not the move-to-solana compiler. Currently, test harness invokes move-cli with relative path argument to -p option. I'll think how to test move-cli with absolute paths given to the -p option.

I'm thinking about adding more directives to test harness, for example via comments in Move.toml file, for specifying absolute or incorrect paths to Move packages. In my view this is inappropriate to have such facility in tests for move-to-solana package, as this package is not related to move-cli. Nevertheless, I'll add it for the sake of better coverage. There are many other components in move repository that we do not directly test, and I don't think it's relevant to this PR.

I'm not talking about running move-cli tests with absolute path. But we were able to test the absolute path in cli-tests and now this test gone, so we need this integration testing back, but it is your choice of means here, certainly.

[dmakarov]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

cli-tests tested just one command line option -p of move-mv-llvm-compiler application. This option does not exist in move-to-solana application, as packages and dependencies are handled by move-cli. Like I already said several times above, I don't think it's appropriate to test options of move-cli in move-to-solana package. If we want to test handling of absolute, relative, incorrect paths by move-cli, we should run move-cli tests and check how move-cli handles the paths or anything it should handle there.

[jcivlin]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

cli-tests tested just one command line option -p of move-mv-llvm-compiler application. This option does not exist in move-to-solana application, as packages and dependencies are handled by move-cli. Like I already said several times above, I don't think it's appropriate to test options of move-cli in move-to-solana package. If we want to test handling of absolute, relative, incorrect paths by move-cli, we should run move-cli tests and check how move-cli handles the paths or anything it should handle there.

Indeed you have repeated this many times. But it is one thing what move-cli tests do and another what integration test does. The latter one is close to the real user compiler invocation and knowing what this user will see is important. I do not see a reason for removing the existing cli-tests.

[dmakarov]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

cli-tests tested just one command line option -p of move-mv-llvm-compiler application. This option does not exist in move-to-solana application, as packages and dependencies are handled by move-cli. Like I already said several times above, I don't think it's appropriate to test options of move-cli in move-to-solana package. If we want to test handling of absolute, relative, incorrect paths by move-cli, we should run move-cli tests and check how move-cli handles the paths or anything it should handle there.

Indeed you have repeated this many times. But it is one thing that move-cli tests do and another what integration test does. The latter one is close to the real user compiler invocation and knowing what this user will see is important. I do not see a reason for removing the existing cli-tests.

I don't see tests of move-to-solana package as integration tests. They are tests of move-to-solana application and not the tests of end user experience.

[jcivlin]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

cli-tests tested just one command line option -p of move-mv-llvm-compiler application. This option does not exist in move-to-solana application, as packages and dependencies are handled by move-cli. Like I already said several times above, I don't think it's appropriate to test options of move-cli in move-to-solana package. If we want to test handling of absolute, relative, incorrect paths by move-cli, we should run move-cli tests and check how move-cli handles the paths or anything it should handle there.

Indeed you have repeated this many times. But it is one thing that move-cli tests do and another what integration test does. The latter one is close to the real user compiler invocation and knowing what this user will see is important. I do not see a reason for removing the existing cli-tests.

I don't see tests of move-to-solana package as integration tests. They are tests of move-to-solana application and not the tests of end user experience.

It is difficult to argue opinions, but it is much easier to operate the facts. The fact is that cli-tests was testing -p option as an option provided by the user. Now this test has gone and it should be either restored or somehow this testing should be restored. And the fact that we handle -p in the move-cli testing does not prove that we handle it correctly in the compiler.

[dmakarov]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

cli-tests tested just one command line option -p of move-mv-llvm-compiler application. This option does not exist in move-to-solana application, as packages and dependencies are handled by move-cli. Like I already said several times above, I don't think it's appropriate to test options of move-cli in move-to-solana package. If we want to test handling of absolute, relative, incorrect paths by move-cli, we should run move-cli tests and check how move-cli handles the paths or anything it should handle there.

Indeed you have repeated this many times. But it is one thing that move-cli tests do and another what integration test does. The latter one is close to the real user compiler invocation and knowing what this user will see is important. I do not see a reason for removing the existing cli-tests.

I don't see tests of move-to-solana package as integration tests. They are tests of move-to-solana application and not the tests of end user experience.

It is difficult to argue opinions, but it is much easier to operate the facts. The fact is that cli-tests was testing -p option as an option provided by the user. Now this test has gone and it should be either restored or somehow this testing should be restored. And the fact that we handle -p in the move-cli testing does not prove that we handle it correctly in the compiler.

I explained already, the option -p is not handled by the the compiler (move-to-solana application) in these changes. The option -p is handled by move-cli. The compiler (move-to-solana) receives all paths from move-cli, however move-cli handles the -p option. Once again we do not handle -p in the compiler. Testing how move-cli handles -p is outside of testing move-to-solana crate. Tests of move-to-solana crate are not integration tests, as user should never invoke move-to-solana compiler directly. It's useless on its own for any purpose without Move packages and move-cli processing package metadata. It's irrelevant what cli-tests tested when this PR is concerned, because cli-tests tested application that handled -p on its own. This is not the case with these changes. This back and forth starts looking like trolling. :-)

[jcivlin]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

cli-tests tested just one command line option -p of move-mv-llvm-compiler application. This option does not exist in move-to-solana application, as packages and dependencies are handled by move-cli. Like I already said several times above, I don't think it's appropriate to test options of move-cli in move-to-solana package. If we want to test handling of absolute, relative, incorrect paths by move-cli, we should run move-cli tests and check how move-cli handles the paths or anything it should handle there.

Indeed you have repeated this many times. But it is one thing that move-cli tests do and another what integration test does. The latter one is close to the real user compiler invocation and knowing what this user will see is important. I do not see a reason for removing the existing cli-tests.

I don't see tests of move-to-solana package as integration tests. They are tests of move-to-solana application and not the tests of end user experience.

It is difficult to argue opinions, but it is much easier to operate the facts. The fact is that cli-tests was testing -p option as an option provided by the user. Now this test has gone and it should be either restored or somehow this testing should be restored. And the fact that we handle -p in the move-cli testing does not prove that we handle it correctly in the compiler.

I explained already, the option -p is not handled by the the compiler (move-to-solana application) in these changes. The option -p is handled by move-cli. The compiler (move-to-solana) receives all paths from move-cli, however move-cli handles the -p option. Once again we do not handle -p in the compiler. Testing how move-cli handles -p is outside of testing move-to-solana crate. Tests of move-to-solana crate are not integration tests, as user should never invoke move-to-solana compiler directly. It's useless on its own for any purpose without Move packages and move-cli processing package metadata. It's irrelevant what cli-tests tested when this PR is concerned, because cli-tests tested application that handled -p on its own. This is not the case with these changes. This back and forth starts looking like trolling. :-)

Please, do not repeat yourself, it does not help. I'm a user, I call compiler and provided -p option. Something does not work (from my user experience). I got an error message or something else that I did not expect. Where is the compiler test which anticipates this user experience?

[dmakarov]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

cli-tests tested just one command line option -p of move-mv-llvm-compiler application. This option does not exist in move-to-solana application, as packages and dependencies are handled by move-cli. Like I already said several times above, I don't think it's appropriate to test options of move-cli in move-to-solana package. If we want to test handling of absolute, relative, incorrect paths by move-cli, we should run move-cli tests and check how move-cli handles the paths or anything it should handle there.

Indeed you have repeated this many times. But it is one thing that move-cli tests do and another what integration test does. The latter one is close to the real user compiler invocation and knowing what this user will see is important. I do not see a reason for removing the existing cli-tests.

I don't see tests of move-to-solana package as integration tests. They are tests of move-to-solana application and not the tests of end user experience.

It is difficult to argue opinions, but it is much easier to operate the facts. The fact is that cli-tests was testing -p option as an option provided by the user. Now this test has gone and it should be either restored or somehow this testing should be restored. And the fact that we handle -p in the move-cli testing does not prove that we handle it correctly in the compiler.

I explained already, the option -p is not handled by the the compiler (move-to-solana application) in these changes. The option -p is handled by move-cli. The compiler (move-to-solana) receives all paths from move-cli, however move-cli handles the -p option. Once again we do not handle -p in the compiler. Testing how move-cli handles -p is outside of testing move-to-solana crate. Tests of move-to-solana crate are not integration tests, as user should never invoke move-to-solana compiler directly. It's useless on its own for any purpose without Move packages and move-cli processing package metadata. It's irrelevant what cli-tests tested when this PR is concerned, because cli-tests tested application that handled -p on its own. This is not the case with these changes. This back and forth starts looking like trolling. :-)

Please, do not repeat yourself, it does not help. I'm a user, I call compiler and provided -p option. Something does not work (from my user experience). I got an error message or something else that I did not expect. Where is the compiler test which anticipates this user experience?

How do you call the compiler? Where do you provide the option -p?

[jcivlin]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

cli-tests tested just one command line option -p of move-mv-llvm-compiler application. This option does not exist in move-to-solana application, as packages and dependencies are handled by move-cli. Like I already said several times above, I don't think it's appropriate to test options of move-cli in move-to-solana package. If we want to test handling of absolute, relative, incorrect paths by move-cli, we should run move-cli tests and check how move-cli handles the paths or anything it should handle there.

Indeed you have repeated this many times. But it is one thing that move-cli tests do and another what integration test does. The latter one is close to the real user compiler invocation and knowing what this user will see is important. I do not see a reason for removing the existing cli-tests.

I don't see tests of move-to-solana package as integration tests. They are tests of move-to-solana application and not the tests of end user experience.

It is difficult to argue opinions, but it is much easier to operate the facts. The fact is that cli-tests was testing -p option as an option provided by the user. Now this test has gone and it should be either restored or somehow this testing should be restored. And the fact that we handle -p in the move-cli testing does not prove that we handle it correctly in the compiler.

I explained already, the option -p is not handled by the the compiler (move-to-solana application) in these changes. The option -p is handled by move-cli. The compiler (move-to-solana) receives all paths from move-cli, however move-cli handles the -p option. Once again we do not handle -p in the compiler. Testing how move-cli handles -p is outside of testing move-to-solana crate. Tests of move-to-solana crate are not integration tests, as user should never invoke move-to-solana compiler directly. It's useless on its own for any purpose without Move packages and move-cli processing package metadata. It's irrelevant what cli-tests tested when this PR is concerned, because cli-tests tested application that handled -p on its own. This is not the case with these changes. This back and forth starts looking like trolling. :-)

Please, do not repeat yourself, it does not help. I'm a user, I call compiler and provided -p option. Something does not work (from my user experience). I got an error message or something else that I did not expect. Where is the compiler test which anticipates this user experience?

How do you call the compiler? Where do you provide the option -p?

As an option in the command line.

[dmakarov]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

cli-tests tested just one command line option -p of move-mv-llvm-compiler application. This option does not exist in move-to-solana application, as packages and dependencies are handled by move-cli. Like I already said several times above, I don't think it's appropriate to test options of move-cli in move-to-solana package. If we want to test handling of absolute, relative, incorrect paths by move-cli, we should run move-cli tests and check how move-cli handles the paths or anything it should handle there.

Indeed you have repeated this many times. But it is one thing that move-cli tests do and another what integration test does. The latter one is close to the real user compiler invocation and knowing what this user will see is important. I do not see a reason for removing the existing cli-tests.

I don't see tests of move-to-solana package as integration tests. They are tests of move-to-solana application and not the tests of end user experience.

It is difficult to argue opinions, but it is much easier to operate the facts. The fact is that cli-tests was testing -p option as an option provided by the user. Now this test has gone and it should be either restored or somehow this testing should be restored. And the fact that we handle -p in the move-cli testing does not prove that we handle it correctly in the compiler.

I explained already, the option -p is not handled by the the compiler (move-to-solana application) in these changes. The option -p is handled by move-cli. The compiler (move-to-solana) receives all paths from move-cli, however move-cli handles the -p option. Once again we do not handle -p in the compiler. Testing how move-cli handles -p is outside of testing move-to-solana crate. Tests of move-to-solana crate are not integration tests, as user should never invoke move-to-solana compiler directly. It's useless on its own for any purpose without Move packages and move-cli processing package metadata. It's irrelevant what cli-tests tested when this PR is concerned, because cli-tests tested application that handled -p on its own. This is not the case with these changes. This back and forth starts looking like trolling. :-)

Please, do not repeat yourself, it does not help. I'm a user, I call compiler and provided -p option. Something does not work (from my user experience). I got an error message or something else that I did not expect. Where is the compiler test which anticipates this user experience?

How do you call the compiler? Where do you provide the option -p?

As an option in the command line.

As an option to what? Can you show the exact command line?

[jcivlin]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

cli-tests tested just one command line option -p of move-mv-llvm-compiler application. This option does not exist in move-to-solana application, as packages and dependencies are handled by move-cli. Like I already said several times above, I don't think it's appropriate to test options of move-cli in move-to-solana package. If we want to test handling of absolute, relative, incorrect paths by move-cli, we should run move-cli tests and check how move-cli handles the paths or anything it should handle there.

Indeed you have repeated this many times. But it is one thing that move-cli tests do and another what integration test does. The latter one is close to the real user compiler invocation and knowing what this user will see is important. I do not see a reason for removing the existing cli-tests.

I don't see tests of move-to-solana package as integration tests. They are tests of move-to-solana application and not the tests of end user experience.

It is difficult to argue opinions, but it is much easier to operate the facts. The fact is that cli-tests was testing -p option as an option provided by the user. Now this test has gone and it should be either restored or somehow this testing should be restored. And the fact that we handle -p in the move-cli testing does not prove that we handle it correctly in the compiler.

I explained already, the option -p is not handled by the the compiler (move-to-solana application) in these changes. The option -p is handled by move-cli. The compiler (move-to-solana) receives all paths from move-cli, however move-cli handles the -p option. Once again we do not handle -p in the compiler. Testing how move-cli handles -p is outside of testing move-to-solana crate. Tests of move-to-solana crate are not integration tests, as user should never invoke move-to-solana compiler directly. It's useless on its own for any purpose without Move packages and move-cli processing package metadata. It's irrelevant what cli-tests tested when this PR is concerned, because cli-tests tested application that handled -p on its own. This is not the case with these changes. This back and forth starts looking like trolling. :-)

Please, do not repeat yourself, it does not help. I'm a user, I call compiler and provided -p option. Something does not work (from my user experience). I got an error message or something else that I did not expect. Where is the compiler test which anticipates this user experience?

How do you call the compiler? Where do you provide the option -p?

As an option in the command line.

As an option to what? Can you show the exact command line?

Whatever of solana architecture will be with this change and -p as an absolute path to the package.

move build --arch solana -p abs_path - something like this, I guess.

[dmakarov]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

cli-tests tested just one command line option -p of move-mv-llvm-compiler application. This option does not exist in move-to-solana application, as packages and dependencies are handled by move-cli. Like I already said several times above, I don't think it's appropriate to test options of move-cli in move-to-solana package. If we want to test handling of absolute, relative, incorrect paths by move-cli, we should run move-cli tests and check how move-cli handles the paths or anything it should handle there.

Indeed you have repeated this many times. But it is one thing that move-cli tests do and another what integration test does. The latter one is close to the real user compiler invocation and knowing what this user will see is important. I do not see a reason for removing the existing cli-tests.

I don't see tests of move-to-solana package as integration tests. They are tests of move-to-solana application and not the tests of end user experience.

It is difficult to argue opinions, but it is much easier to operate the facts. The fact is that cli-tests was testing -p option as an option provided by the user. Now this test has gone and it should be either restored or somehow this testing should be restored. And the fact that we handle -p in the move-cli testing does not prove that we handle it correctly in the compiler.

I explained already, the option -p is not handled by the the compiler (move-to-solana application) in these changes. The option -p is handled by move-cli. The compiler (move-to-solana) receives all paths from move-cli, however move-cli handles the -p option. Once again we do not handle -p in the compiler. Testing how move-cli handles -p is outside of testing move-to-solana crate. Tests of move-to-solana crate are not integration tests, as user should never invoke move-to-solana compiler directly. It's useless on its own for any purpose without Move packages and move-cli processing package metadata. It's irrelevant what cli-tests tested when this PR is concerned, because cli-tests tested application that handled -p on its own. This is not the case with these changes. This back and forth starts looking like trolling. :-)

Please, do not repeat yourself, it does not help. I'm a user, I call compiler and provided -p option. Something does not work (from my user experience). I got an error message or something else that I did not expect. Where is the compiler test which anticipates this user experience?

How do you call the compiler? Where do you provide the option -p?

As an option in the command line.

As an option to what? Can you show the exact command line?

Whatever of solana architecture will be with this change and -p as an absolute path to the package.

move build --arch solana -p abs_path - something like this, I guess.

Correct. move-cli processes -p option. The compiler has nothing to do with this. Compiler expects move-cli to handle -p appropriately. Testing move-cli is outside of the scope for the crate move-to-solana. Integration tests and user user experience tests are outside of move-to-solana crate, when move-cli (move build in this case) command line options are concerned.

[jcivlin]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

cli-tests tested just one command line option -p of move-mv-llvm-compiler application. This option does not exist in move-to-solana application, as packages and dependencies are handled by move-cli. Like I already said several times above, I don't think it's appropriate to test options of move-cli in move-to-solana package. If we want to test handling of absolute, relative, incorrect paths by move-cli, we should run move-cli tests and check how move-cli handles the paths or anything it should handle there.

Indeed you have repeated this many times. But it is one thing that move-cli tests do and another what integration test does. The latter one is close to the real user compiler invocation and knowing what this user will see is important. I do not see a reason for removing the existing cli-tests.

I don't see tests of move-to-solana package as integration tests. They are tests of move-to-solana application and not the tests of end user experience.

It is difficult to argue opinions, but it is much easier to operate the facts. The fact is that cli-tests was testing -p option as an option provided by the user. Now this test has gone and it should be either restored or somehow this testing should be restored. And the fact that we handle -p in the move-cli testing does not prove that we handle it correctly in the compiler.

I explained already, the option -p is not handled by the the compiler (move-to-solana application) in these changes. The option -p is handled by move-cli. The compiler (move-to-solana) receives all paths from move-cli, however move-cli handles the -p option. Once again we do not handle -p in the compiler. Testing how move-cli handles -p is outside of testing move-to-solana crate. Tests of move-to-solana crate are not integration tests, as user should never invoke move-to-solana compiler directly. It's useless on its own for any purpose without Move packages and move-cli processing package metadata. It's irrelevant what cli-tests tested when this PR is concerned, because cli-tests tested application that handled -p on its own. This is not the case with these changes. This back and forth starts looking like trolling. :-)

Please, do not repeat yourself, it does not help. I'm a user, I call compiler and provided -p option. Something does not work (from my user experience). I got an error message or something else that I did not expect. Where is the compiler test which anticipates this user experience?

How do you call the compiler? Where do you provide the option -p?

As an option in the command line.

As an option to what? Can you show the exact command line?

Whatever of solana architecture will be with this change and -p as an absolute path to the package. move build --arch solana -p abs_path - something like this, I guess.

Correct. move-cli processes -p option. The compiler has nothing to do with this. Compiler expects move-cli to handle -p appropriately. Testing move-cli is outside of the scope for the crate move-to-solana. Integration tests and user user experience tests are outside of move-to-solana crate, when move-cli (move build in this case) command line options are concerned.

In your own words the current move-cli does not test abs path. And you describe the process how compiler is invoked whereas such test (cli-tests) is agnostic to this and replicates the user experience. If we build compiler differently (and we actually do with your change) this test stays.. You may not need to create this test if it did not exist, but once it was there it should be there.

[dmakarov]

And absolute path and incorrect path, whether they handled or not by move-cli unit tests.

Did the original tests of -p that I removed include a test case with an incorrect path? If not, please, feel free to add that later.

I likewise anyone can add it later. As I said at the very beginning I want to know what is the test plan for this PR.

What was the reason for removing cli-tests, if -p was tested here as rel and abs too? It does not matter whether -p handled by move-cli, the test should stand.

cli-tests tested just one command line option -p of move-mv-llvm-compiler application. This option does not exist in move-to-solana application, as packages and dependencies are handled by move-cli. Like I already said several times above, I don't think it's appropriate to test options of move-cli in move-to-solana package. If we want to test handling of absolute, relative, incorrect paths by move-cli, we should run move-cli tests and check how move-cli handles the paths or anything it should handle there.

Indeed you have repeated this many times. But it is one thing that move-cli tests do and another what integration test does. The latter one is close to the real user compiler invocation and knowing what this user will see is important. I do not see a reason for removing the existing cli-tests.

I don't see tests of move-to-solana package as integration tests. They are tests of move-to-solana application and not the tests of end user experience.

It is difficult to argue opinions, but it is much easier to operate the facts. The fact is that cli-tests was testing -p option as an option provided by the user. Now this test has gone and it should be either restored or somehow this testing should be restored. And the fact that we handle -p in the move-cli testing does not prove that we handle it correctly in the compiler.

I explained already, the option -p is not handled by the the compiler (move-to-solana application) in these changes. The option -p is handled by move-cli. The compiler (move-to-solana) receives all paths from move-cli, however move-cli handles the -p option. Once again we do not handle -p in the compiler. Testing how move-cli handles -p is outside of testing move-to-solana crate. Tests of move-to-solana crate are not integration tests, as user should never invoke move-to-solana compiler directly. It's useless on its own for any purpose without Move packages and move-cli processing package metadata. It's irrelevant what cli-tests tested when this PR is concerned, because cli-tests tested application that handled -p on its own. This is not the case with these changes. This back and forth starts looking like trolling. :-)

Please, do not repeat yourself, it does not help. I'm a user, I call compiler and provided -p option. Something does not work (from my user experience). I got an error message or something else that I did not expect. Where is the compiler test which anticipates this user experience?

How do you call the compiler? Where do you provide the option -p?

As an option in the command line.

As an option to what? Can you show the exact command line?

Whatever of solana architecture will be with this change and -p as an absolute path to the package. move build --arch solana -p abs_path - something like this, I guess.

Correct. move-cli processes -p option. The compiler has nothing to do with this. Compiler expects move-cli to handle -p appropriately. Testing move-cli is outside of the scope for the crate move-to-solana. Integration tests and user user experience tests are outside of move-to-solana crate, when move-cli (move build in this case) command line options are concerned.

In your own words the current move-cli does not test abs path. And you describe the process how compiler is invoked whereas such test (cli-tests) is agnostic to this and replicates the user experience.

This is not correct, because cli-tests tested an application that handled -p option. It was the functionality of move-mv-llvm-compiler application to accept -p option and handle paths given in this option. This is no longer the case. We should have integration tests, testing move build --arch solana -p path with absolute, relative and incorrect paths. However, these tests are outside the scope of the tests for the crate move-to-solana. I can add tests that test move-to-solana application command line with input files specified as relative, absolute and non-existing paths. There were no such tests previously, and I don't think they must be added in this PR.

If we build compiler differently (and we actually do with your change) this test stays.. You may not need to create this test if it did not exist, but once it was there it should be there.

I don't quite understand this. The test that existed did not test paths of input files to the compiler, but handling of paths specified in -p option. This option is no longer handled by the compiler, so those tests are irrelevant for this PR.

[jcivlin]

I can add tests that test move-to-solana application command line with input files specified as relative, absolute and non-existing paths. There were no such tests previously, and I don't think they must be added in this PR.

Yes, cli-tests were testing abs path. I planned to add a test with incorrect path. You do not need to add what did not exist, but cli-tests were testing the abs and rel paths.

The cli-tests were testing -p in the user command line, in particular if the provided path was correct - pointing to the Move.toml - then the dependency could be resolved (potentially the dependency resolution can fail for many reasons, but cli-tests were testing only the successful scenario). In other words, cli-tests were testing not just what it is in the -p, but also the actual dependency resolution mechanism (maybe cli-tests is a misnomer or misleading). So If you have cli-tests replaced by something then it is fine to remove. But definitely they are not replaced since we cannot even use the abs path. I'm not asking to add the new tests but removing the existing testing would be misfortunate.

[dmakarov]

Here https://github.com/solana-labs/move/blob/a319615dfdd0ad2d6537679c9962767940542671/language/tools/move-mv-llvm-compiler/tests/cli-tests.rs#L109

cli-tests tested move-mv-llvm-compiler accepting a path to a package. Moreover, the path was not even to a package, but to a directory containing Move.toml, while the Move source file was outside that directory. Yes, the user experience with move-cli is similar, but the whole flow of what cli-tests tested is different. It was not agnostic to how the compiler is invoked, because the test did invoke the compiler with specific options, and specific layout of source files and directory containing Move.toml. What functionality of cli-test do you want to retain with this PR? The application move-to-solana does not accept paths to packages. I agree that integration tests of running move-cli build -p are necessary with different sets of paths. I do not agree that such tests are part of this PR. I also do not agree that cli-tests that I removed tested the same user experience.

[dmakarov]

I can add tests that test move-to-solana application command line with input files specified as relative, absolute and non-existing paths. There were no such tests previously, and I don't think they must be added in this PR.

Yes, cli-tests were testing abs path. I planned to add a test with incorrect path. You do not need to add what did not exist, but cli-tests were testing the abs and rel paths.

The cli-tests were testing -p in the user command line, in particular if the provided path was correct - pointing to the Move.toml - then the dependency could be resolved (potentially the dependency resolution can fail for many reasons, but cli-tests were testing only the successful scenario). In other words, cli-tests were testing not just what it is in the -p, but also the actual dependency resolution mechanism (maybe cli-tests is a misnomer or misleading). So If you have cli-tests replaced by something then it is fine to remove. But definitely they are not replaced since we cannot even use the abs path. I'm not asking to add the new tests but removing the existing testing would be misfortunate.

That dependency resolution mechanism was used by your implementation of handling paths to packages by the compiler and was part of the compiler. This mechanism is now not part of the compiler, but part of move-cli, as it should be. Testing the dependency resolution is important and must be done. However, it's not part of this PR and not part of move-to-solana crate.

[dmakarov]

For what it's worth (not a replacement for testing) I see that move build handles both relative and absolute paths to packages correctly, and invokes the move-to-solana compiler with correct paths in both cases. Whatever issues with dependency resolution mechanism you had it might be related to its use in move-mv-llvm-compiler and the very specific layout of test cases, where Move source file is in one directory and Move.toml is in subdirectory of this directory. That's why I say the flow of using move build -p is different than with move-to-llvm-compiler -p.

I'd like to see other reviewers comments on this issue.

[jcivlin]

cli-tests tested move-mv-llvm-compiler accepting a path to a package. Moreover, the path was not even to a package, but to a directory containing Move.toml, while the Move source file was outside that directory.

I do not see a point. Move.toml may be omitted in the path provided in -p.

Yes, the user experience with move-cli is similar, but the whole flow of what cli-tests tested is different. It was not agnostic to how the compiler is invoked, because the test did invoke the compiler with specific options, and specific layout of source files and directory containing Move.toml.

This test works like this. For the packages resolution there is a convention of keeping the package source in some subdirs of path provided in -p. This is rigid since we use move-package for dependency resolution. But the source is just a path defined in -c.

What functionality of cli-test do you want to retain with this PR? The application move-to-solana does not accept paths to packages. I agree that integration tests of running move-cli build -p are necessary with different sets of paths. I do not agree that such tests are part of this PR. I also do not agree that cli-tests that I removed tested the same user experience.

Actually, since the flow will change once the move-cli will be in the place I want more than just test. I want to retain the same functionality as cli-tests demoed:

• one can compile own source pointed by -c with dependency definitions (Move.toml) pointed by -p.

So testing this becomes essential.

As I mentioned upfront, I'm ok if this testing will be added in another PR.

[jcivlin]

That dependency resolution mechanism was used by your implementation of handling paths to packages by the compiler and was part of the compiler. This mechanism is now not part of the compiler, but part of move-cli, as it should be. Testing the dependency resolution is important and must be done. However, it's not part of this PR and not part of move-to-solana crate.

It is a repetitious argument here that "this test does not belong to this PR". But this PR (may) change the compiler behavior, in particular, ability to compile the user source, pointed by -c with dependency definitions pointed by -p. Why? Because we change the mechanics of processing options and processing dependency (package.rs has gone). So we need to test it. Rumination of which crate this test should belong to does't help to convince me that we should not test at all.

[dmakarov]

cli-tests tested move-mv-llvm-compiler accepting a path to a package. Moreover, the path was not even to a package, but to a directory containing Move.toml, while the Move source file was outside that directory.

I do not see a point. Move.toml may be omitted in the path provided in -p.

What was the point adding it to the test case used in cli-tests? If it can be omitted what purpose did it serve?

Yes, the user experience with move-cli is similar, but the whole flow of what cli-tests tested is different. It was not agnostic to how the compiler is invoked, because the test did invoke the compiler with specific options, and specific layout of source files and directory containing Move.toml.

This test works like this. For the packages resolution there is a convention of keeping the package source in some subdirs of path provided in -p. This is rigid since we use move-package for dependency resolution. But the source is just a path defined in -c.

That's how your implementation of package resolution works in move-mv-llvm-compiler. It also doesn't seem to hold for the test files of cli-test

$ ls cli-tests
basic-coin  basic-coin-build  basic-coin.move
$ ls cli-tests/basic-coin
Move.toml

Here the source is not in subdir of path provided in -p, if -p points to directory of Move.toml.

What functionality of cli-test do you want to retain with this PR? The application move-to-solana does not accept paths to packages. I agree that integration tests of running move-cli build -p are necessary with different sets of paths. I do not agree that such tests are part of this PR. I also do not agree that cli-tests that I removed tested the same user experience.

Actually, since the flow will change once the move-cli will be in the place I want more than just test. I want to retain the same functionality as cli-tests demoed:

* one can compile own source pointed by -c with dependency definitions (Move.toml) pointed by -p.

move-cli doesn't allow this and doesn't accept -c.

So testing this becomes essential.

As I mentioned upfront, I'm ok if this testing will be added in another PR.

Sure, we can add that later.

[dmakarov]

That dependency resolution mechanism was used by your implementation of handling paths to packages by the compiler and was part of the compiler. This mechanism is now not part of the compiler, but part of move-cli, as it should be. Testing the dependency resolution is important and must be done. However, it's not part of this PR and not part of move-to-solana crate.

It is a repetitious argument here that "this test does not belong to this PR". But this PR (may) change the compiler behavior, in particular, ability to compile the user source, pointed by -c with dependency definitions pointed by -p. Why? Because we change the mechanics of processing options and processing dependency (package.rs has gone). So we need to test it. Rumination of which crate this test should belong to does't help to convince me that we should not test at all.

Throughout this entire discussion I maintained that we must test this, except the testing of move-cli doesn't belong in move-to-solana crate or this PR. Please, point me to what I said that might be consider as an attempt to convince you that we should not test at all.

[dmakarov]

That dependency resolution mechanism was used by your implementation of handling paths to packages by the compiler and was part of the compiler. This mechanism is now not part of the compiler, but part of move-cli, as it should be. Testing the dependency resolution is important and must be done. However, it's not part of this PR and not part of move-to-solana crate.

It is a repetitious argument here that "this test does not belong to this PR". But this PR (may) change the compiler behavior, in particular, ability to compile the user source, pointed by -c with dependency definitions pointed by -p.

By the way, this ability is gone with this change. move-cli doesn't accept -c and move-to-solana doesn't accept -p. Again, user should never need to invoke move-to-solana directly. The Move build environment model is to use a CLI tool such as move build, move test, etc (in case of aptos it's aptos move build).

Why? Because we change the mechanics of processing options and processing dependency (package.rs has gone). So we need to test it. Rumination of which crate this test should belong to does't help to convince me that we should not test at all.

[dmakarov]

I think the follow up to this PR should be changes that make move test --solana run Move unit tests on Solana VM, similar to how move test --evm runs Move unit tests on Ethereum VM. Then we will have nearly complete user experience for building Move packages and testing them on Solana VM.

[jcivlin]

cli-tests tested move-mv-llvm-compiler accepting a path to a package. Moreover, the path was not even to a package, but to a directory containing Move.toml, while the Move source file was outside that directory.

I do not see a point. Move.toml may be omitted in the path provided in -p.

What was the point adding it to the test case used in cli-tests? If it can be omitted what purpose did it serve?

The nameMove.toml may be omitted n the path set by -p. It may be the file name or just the directory that contains Move.toml.

Yes, the user experience with move-cli is similar, but the whole flow of what cli-tests tested is different. It was not agnostic to how the compiler is invoked, because the test did invoke the compiler with specific options, and specific layout of source files and directory containing Move.toml.

This test works like this. For the packages resolution there is a convention of keeping the package source in some subdirs of path provided in -p. This is rigid since we use move-package for dependency resolution. But the source is just a path defined in -c.

That's how your implementation of package resolution works in move-mv-llvm-compiler. It also doesn't seem to hold for the test files of cli-test

$ ls cli-tests
basic-coin  basic-coin-build  basic-coin.move
$ ls cli-tests/basic-coin
Move.toml

What does not hold? I just explained that there is a diff between the structure of the package and the user source. The former is rigid (or as move-package defined) and the latter is free. So in this test -c points to the user source code, which may be anywhere. Let say a user has many files that all use the same Move.toml.

Here the source is not in subdir of path provided in -p, if -p points to directory of Move.toml.

I just explained this.

What functionality of cli-test do you want to retain with this PR? The application move-to-solana does not accept paths to packages. I agree that integration tests of running move-cli build -p are necessary with different sets of paths. I do not agree that such tests are part of this PR. I also do not agree that cli-tests that I removed tested the same user experience.

Actually, since the flow will change once the move-cli will be in the place I want more than just test. I want to retain the same functionality as cli-tests demoed:

* one can compile own source pointed by -c with dependency definitions (Move.toml) pointed by -p.

move-cli doesn't allow this and doesn't accept -c.

It will be bad to drop -c option. The way move build -p dir works is it has the sources in dir/sources and Move.toml in the dir. Then anytime a source (even one file) is moved, the Move.toml must be updated. If both options -p and -c options available Move.toml is yet in a rigid position relative to positioning of the used packages, but the user's source may be freely moved. This also makes compilation of many independent files with similar package dependencies much easier.

So testing this becomes essential. As I mentioned upfront, I'm ok if this testing will be added in another PR.

Sure, we can add that later.

[jcivlin]

Please update the description and mention what options were dropped.

[dmakarov]

cli-tests tested move-mv-llvm-compiler accepting a path to a package. Moreover, the path was not even to a package, but to a directory containing Move.toml, while the Move source file was outside that directory.

I do not see a point. Move.toml may be omitted in the path provided in -p.

What was the point adding it to the test case used in cli-tests? If it can be omitted what purpose did it serve?

The nameMove.toml may be omitted n the path set by -p. It may be the file name or just the directory that contains Move.toml.

Yes, the user experience with move-cli is similar, but the whole flow of what cli-tests tested is different. It was not agnostic to how the compiler is invoked, because the test did invoke the compiler with specific options, and specific layout of source files and directory containing Move.toml.

This test works like this. For the packages resolution there is a convention of keeping the package source in some subdirs of path provided in -p. This is rigid since we use move-package for dependency resolution. But the source is just a path defined in -c.

That's how your implementation of package resolution works in move-mv-llvm-compiler. It also doesn't seem to hold for the test files of cli-test

$ ls cli-tests
basic-coin  basic-coin-build  basic-coin.move
$ ls cli-tests/basic-coin
Move.toml

What does not hold? I just explained that there is a diff between the structure of the package and the user source. The former is rigid (or as move-package defined) and the latter is free. So in this test -c points to the user source code, which may be anywhere. Let say a user has many files that all use the same Move.toml.

Here the source is not in subdir of path provided in -p, if -p points to directory of Move.toml.

I just explained this.

What functionality of cli-test do you want to retain with this PR? The application move-to-solana does not accept paths to packages. I agree that integration tests of running move-cli build -p are necessary with different sets of paths. I do not agree that such tests are part of this PR. I also do not agree that cli-tests that I removed tested the same user experience.

Actually, since the flow will change once the move-cli will be in the place I want more than just test. I want to retain the same functionality as cli-tests demoed:

* one can compile own source pointed by -c with dependency definitions (Move.toml) pointed by -p.

move-cli doesn't allow this and doesn't accept -c.

It will be bad to drop -c option. The way move build -p dir works is it has the sources in dir/sources and Move.toml in the dir. Then anytime a source (even one file) is moved, the Move.toml must be updated. If both options -p and -c options available Move.toml is yet in a rigid position relative to positioning of the used packages, but the user's source may be freely moved. This also makes compilation of many independent files with similar package dependencies much easier.

The option -c is not dropped from the move-to-solana application command line. It is used as usual in other compilers, meaning compile only, do not link.

I would prefer to maintain the behavior of move-cli as users are used to at least for now.

So testing this becomes essential. As I mentioned upfront, I'm ok if this testing will be added in another PR.

Sure, we can add that later.

I updated the description of the PR.

[dmakarov]

Please update the description and mention what options were dropped.

Done.

[jcivlin]

So testing this becomes essential. As I mentioned upfront, I'm ok if this testing will be added in another PR.

Sure, we can add that later.

[dmakarov]

@ksolana @nvjle @brson more comments?

[jcivlin]

The option -c is not dropped from the move-to-solana application command line. It is used as usual in other compilers, meaning compile only, do not link.

We are talking here about --compile. It allowed to build from the same source either against Diem or Aptos stdlib, for example.

[dmakarov]

The option -c is not dropped from the move-to-solana application command line. It is used as usual in other compilers, meaning compile only, do not link.

We are talking here about --compile. It allowed to build from the same source either against Diem or Aptos stdlib, for example.

Why --compile should control which standard library to use? Can't this be specified in Move.toml?

With this change I'd like to have tools work for compiling Move packages to Solana BPF .so files end to end without any strange options or some unconventional interpretation of how Move packages could be organized. I also would like us to stop replicating functionality from other parts of Move repository, such as package dependency resolution. As useful as the latter might be, I'd like to move forward with tooling in-line with what already exists and somewhat familiar to the user community. In particular, stand-alone move-mv-llvm-compiler is not needed at all in this environment. With the changes in this PR we'll still use move-to-solana standalone application for compilation testing, but I would like to possibly eliminate this too, and use Move unit tests for testing purposes. Then we can have integration tests and test actual user experience.

[dmakarov]

I thought about adding an environment variable that move-to-solana would recognize and accept additional options if for some reason we need to do that and cannot pass them via move-cli. This is how rustc compiler accepts additional options when run by cargo. RUSTFLAGS environment variable allows to specify additional options to rustc compiler controlled by cargo.

[jcivlin]

Why --compile should control which standard library to use? Can't this be specified in Move.toml?

No, it does not. Option -p selects the dependency and -c selects the source(s) of compilation. This is a "natural" extension of the api we use:

run_model_builder_with_options_and_compilation_flags(sources, deps, options, flags)

Basically, compilation of the package is a case with an empty sources and all package sources set in deps.

[jcivlin]

I also would like us to stop replicating functionality from other parts of Move repository

I'm for this, but it is a policy that we have to subscribe to. There are bugs in these tools and we will need to do online patching.

[jcivlin]

I thought about adding an environment variable that move-to-solana would recognize and accept additional options if for some reason we need to do that and cannot pass them via move-cli. This is how rustc compiler accepts additional options when run by cargo. RUSTFLAGS environment variable allows to specify additional options to rustc compiler controlled by cargo.

move build and aptos move compile have different interfaces, so the Pandora's box has been open and we would not gain much by pretending virginity. I'd add the options as we need them, at least internal ones that help to debug or test.

[dmakarov]

I also would like us to stop replicating functionality from other parts of Move repository

I'm for this, but it is a policy that we have to subscribe to. There are bugs in these tools and we will need to do online patching.

This is a very good point. Thank you. We should think about this. Out of curiosity when you added -p, —sdtlib options and made move-mv-llvm-compiler depend on crates move-package, move-cli and others were you not concerned they’re buggy? You haven’t subscribed us to online patching of those crates at that time?

[jcivlin]

I also would like us to stop replicating functionality from other parts of Move repository

I'm for this, but it is a policy that we have to subscribe to. There are bugs in these tools and we will need to do online patching.

This is a very good point. Thank you. We should think about this. Out of curiosity when you added -p, —sdtlib options and made move-mv-llvm-compiler depend on crates move-package, move-cli and others were you not concerned they’re buggy?

Of course I was concerned. It is the reason I rewrote some functions. My code in package.rs is about 200 lines (it is longer for move-mv-llvm-cmpiler, but I shortened it to 200 lines while porting to Aptos). It would be even shorter if I did not need to rewrite some functions. I mentioned this a few times and once in the last meeting, you replied "Everything is buggy" - so all this is known. One clear bug is in the hex addr - you are aware of this one. Another is more subtle, it is in the dependency processing and we may not ever hit it if compile only against the one level deep packages. Although I'm not calling this 100% bug, because even with the nested dependency it is possible to write all Move.toml paths in a way attuned to this (buggy) function

You haven’t subscribed us to online patching of those crates at that time?

In my solution this subscription was not required, my patches are outside of the crates.

[dmakarov]

I also would like us to stop replicating functionality from other parts of Move repository

I'm for this, but it is a policy that we have to subscribe to. There are bugs in these tools and we will need to do online patching.

This is a very good point. Thank you. We should think about this. Out of curiosity when you added -p, —sdtlib options and made move-mv-llvm-compiler depend on crates move-package, move-cli and others were you not concerned they’re buggy?

Of course I was concerned. It is the reason I rewrote some functions. My code in package.rs is about 200 lines (it is longer for move-mv-llvm-cmpiler, but I shortened it to 200 lines while porting to Aptos). It would be even shorter if I did not need to rewrite some functions. I mentioned this a few times and once in the last meeting, you replied "Everything is buggy" - so all this is known. One clear bug is in the hex addr - you are aware of this one. Another is more subtle, it is in the dependency processing and we may not ever hit it if compile only against the one level deep packages. Although I'm not calling this 100% bug, because even with the nested dependency it is possible to write all Move.toml paths in a way attuned to this (buggy) function

You haven’t subscribed us to online patching of those crates at that time?

In my solution this subscription was not required, my patches are outside of the crates.

Please, correct me if I’m wrong:

If this PR is not merged

1. move-mv-llvm-compiler will keep dependencies on crates move-cli, move-packages, and others that you added when you implemented -p and —stdlib options;
2. if more bugs are found in crates move-cli, move-packages, and others, dependencies on which you added, you will fix the bugs by reimplementing the corresponding functionality from those crates inside the move-mv-llvm-compiler, instead of fixing them in the crates that have the bugs;
3. in your terms the item 2 above is not a subscription to online patching because instead of patching bugs in-place the functionality is reimplemented inside move-mv-llvm-compiler.

Also from your other comment I understand that merging this PR opens a Pandora’s box but adding crates move-package, move-cli as dependencies to move-mv-llvm-compiler crate was not opening Pandora’s box.

Is my understanding correct?