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

Make it easier for users to get nightly builds #13802

Closed
6 tasks
cameel opened this issue Dec 13, 2022 · 12 comments
Closed
6 tasks

Make it easier for users to get nightly builds #13802

cameel opened this issue Dec 13, 2022 · 12 comments
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. medium effort Default level of effort medium impact Default level of impact should have We like the idea but it’s not important enough to be a part of the roadmap. solcbin stale The issue/PR was marked as stale because it has been open for too long.

Comments

@cameel
Copy link
Member

cameel commented Dec 13, 2022

We provide nightly builds in solc-bin but these are emscripten-only. The static build artifacts on develop in the main repo can also be used as nightlies but this is not apparent to users and explaining how to get them is not very straightforward. There are a few things we could do to improve this situation:

  • Add b_ubu_static, b_win and b_osx runs to the nightly workflow. This will make it easier for users to locate nightlies when they look into our CI.
  • Expand the solc-bin README
    • Document how to get nightlies from CircleCI. Point out that b_ubu_static, b_win, b_osx and b_ems jobs produce static binaries and upload them as artifacts. Explain how to find the right workflow in CircleCI.
    • Add more info about the structure of solc-bin. What is in each dir and that there are lists with meta information. Use the info that's already available in docs on Static Binaries.
    • Add links to latest binaries and nightlies. bin/soljson-nightly.js, bin/soljson-latest.js, etc. Their locations do not change so it's possible to have a direct link in the README.
  • Make static builds available at https://binaries.soliditylang.org
    • Add a job to the nightly workflow to upload the static binaries to our S3 bucket.
      • Use the aws s3 utility (like we do in s3-mirror workflow and sync-s3.sh script).
      • The simplest option is to upload to the solc-bin bucket under some path that's not currently in use (e.g. nightly/). You'll just need to exclude it in the S3 sync script to ensure the script does not delete the files when syncing. There should also be a check that this path does not exist in solc-bin.
      • Using a separate bucket is also an option but then the content will not be served from under the binaries.soliditylang.org domain. This might still be acceptable given that we'll have links in the README though.
      • We only need the latest nightly - no need to version them. They should be available at a predefined path that does not change. Each new upload can overwrite the existing nightlies. The old binaries are still available in CI for some time.
    • Add static links to these new nightlies in the README.
@cameel cameel added solcbin medium effort Default level of effort medium impact Default level of impact should have We like the idea but it’s not important enough to be a part of the roadmap. labels Dec 13, 2022
@cameel cameel added this to Triage in Solidity via automation Dec 13, 2022
@cameel cameel removed this from Triage in Solidity Dec 13, 2022
@ekpyron
Copy link
Member

ekpyron commented Dec 14, 2022

For the record: thinking about this, it's very dangerous to point people to the develop builds as nightlies. If anyone ever has the crazy idea of actually deploying something built by any of those builds, it'll be hell for e.g. sourcify to verify it. So we should indeed make sure that we only point to nightlies that stem from the same commit hash as the emscripten builds we're archiving.

@cameel
Copy link
Member Author

cameel commented Dec 14, 2022

But anyone can do that at any time by just building it on his own so Sourcify already has that problem. Someone could even edit the bytecode to put a completely made up version in there or replace the actual version with something wrong.

Even if it did not happen yet, it can happen at any time and Sourcify has to be able to deal with that in some way. IMO just telling the user that the contract comes from an unrecognized compiler version would be fine. These will be very rare, fringe cases anyway.

@kuzdogan What's your opinion? What does Sourcify do now when it cannot find a matching compiler or the version embedded in the bytecode is missing of is something crazy?

@kuzdogan
Copy link
Member

kuzdogan commented Dec 15, 2022

We actually don't look for the version in the bytecode but from the metadata. We get the compilers from:

I see none work for nightlies. I deployed a test contract on Sepolia at 0x4d041e01b59D324D9b2b1f7d6Dcb4F7608e6fd10 with 0.8.17-nightly.2022.8.9+commit.6b605 24c via Remix.

I see its metadata is outputting 0.8.17-ci.2022.8.9+commit.6b60524c which is different than how it's saved in solc-bin, which is 0.8.17-nightly.2022.8.9+commit.6b60524c. The CBOR encoding is also 0.8.17-ci.2022.8.9+commit.6b60524c.

I can confirm solc-js can fetch 0.8.17-nightly.2022.8.9+commit.6b60524c but not 0.8.17-ci.2022.8.9+commit.6b60524c. So we can verify if this discrepancy is fixed. Edit: We'll handle the cases when it's -ci. instead of -nightly as this is what it is so far.

Attaching the metadata and source here.
1_Storage.sol.txt
Storage_metadata.json.txt

@cameel
Copy link
Member Author

cameel commented Dec 15, 2022

I see its metadata is outputting 0.8.17-ci.2022.8.9+commit.6b60524c which is different than how it's saved in solc-bin, which is 0.8.17-nightly.2022.8.9+commit.6b60524c. The CBOR encoding is also 0.8.17-ci.2022.8.9+commit.6b60524c.

Yeah, that was a bug on our side that went unnoticed for quite a while. Or actually, we thought we fixed it but it turned out that the build script in the main repo we overwriting the prerelease string we set in solc-bin. This was fixed for recent builds by #13581/ethereum/solc-bin#123. We could add aliases for names matching version strings if that helps you but we also have that problem with early binaries, which had pretty wild version strings - so not sure it matters that much.

I see none work for nightlies.

ok, so this means that you can already handle versions that can't be fetched from solc-bin in a graceful way, right? I.e. by dislaying some error message?

@cameel
Copy link
Member Author

cameel commented Dec 15, 2022

By the way, we actually have a PR to include the reported versions on the file list. That would slow list generation quite a bit and would complicate the CI because we can't really execute binaries for all platforms on the same machine, so we were reluctant to merge it so far but I wonder it this would actually be useful to you in case of these name/version discrepancies: ethereum/solc-bin#21.

@kuzdogan
Copy link
Member

ok, so this means that you can already handle versions that can't be fetched from solc-bin in a graceful way, right? I.e. by dislaying some error message?

So yes it shows "Binary couldn't be fetched" or similar, which is the error thrown by the solc-js itself.

But I think we should handle nightlies and this simple fix should do it:
ethereum/sourcify@cee9d72 . What do you mean by wild version strings? I checked them in list.txt and they seem to be pretty consistent. This should fix it right?

By the way, we actually have a PR to include the reported versions on the file list. That would slow list generation quite a bit and would complicate the CI because we can't really execute binaries for all platforms on the same machine, so we were reluctant to merge it so far but I wonder it this would actually be useful to you in case of these name/version discrepancies: ethereum/solc-bin#21.

Sorry, I lack the context, what are the reported versions?

@cameel
Copy link
Member Author

cameel commented Dec 15, 2022

Sorry, I lack the context, what are the reported versions?

By "reported versions" i mean the stuff printed by solc --version or the version() method. That ideally should match the file name, but historically it unfortunately did not.

What do you mean by wild version strings? I checked them in list.txt and they seem to be pretty consistent. This should fix it right?

list.txt has file names, not reported versions. Take a look at releases below 0.4.0 in ethereum/solc-bin#64 (comment) if you want to see what I mean. I checked what all the binaries reported back when I was doing macOS rebuilds and below 0.4.0 you may get stuff like 0.2.0-4dc2445e/.-Emscripten/clang/int linked to libethereum-1.1.0-34716679/.-Emscripten/clang/int.

Versions reported by nightlies actually look more reasonable there but that's only because we temporarily removed pre-0.4 binaries from the repo when we were running out of space on GH pages (they're back now). You can see that on 0.4.1 the version string was wild enough that solc-js was choking on it (I think we either fixed that since then or at least ethereum/solc-js#563 will fix it).

But I think we should handle nightlies and this simple fix should do it:
ethereum/sourcify@cee9d72 .

Well, should be good enough for now but these strings historically were much more complicated and this reminds me that we actually have a mechanism in solc-js for cleaning them - versionToSemver(). We should make it handle the ci variant and then you'll be able to simply use that function and not care how dirty the version string is.

@kuzdogan
Copy link
Member

kuzdogan commented Dec 15, 2022

Now I see thanks a lot @cameel

So yes on our side we handle the cases mostly (and it's pretty rare), and when we can't, there will be a graceful error. Plus, the metadata was only introduced in 0.4.7 anyway, we shouldn't be effected from the wild ones.

@cameel
Copy link
Member Author

cameel commented Dec 16, 2022

We should make it handle the ci variant and then you'll be able to simply use that function and not care how dirty the version string is.

Regarding this, I wanted to report it as a bug in solc-js but then I realized that versionToSemver() should be able to handle these just fine already. It won't translate ci to nightly, but I'm not sure we actually want that - ci is valid for builds that actually come from our main CI and it's hard to distinguish them from solc-bin nightlies. So I guess we just have to accept that for nightlies it can be either ci or nightly.

@kuzdogan
Copy link
Member

I can confirm that solc.loadRemoteVersion('v0.8.17-ci.2022.8.9+commit.6b60524c') is not working but solc.loadRemoteVersion('v0.8.17-nightly.2022.8.9+commit.6b60524c') works and that was the issue for us. I guess we want it for downloading at least? I also don't see any tests for this case in solc-js if this needs to be translated.

@github-actions
Copy link

github-actions bot commented Apr 1, 2023

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Apr 1, 2023
@github-actions
Copy link

github-actions bot commented Apr 8, 2023

Hi everyone! This issue has been automatically closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.

@github-actions github-actions bot added the closed due inactivity The issue/PR was automatically closed due to inactivity. label Apr 8, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. medium effort Default level of effort medium impact Default level of impact should have We like the idea but it’s not important enough to be a part of the roadmap. solcbin stale The issue/PR was marked as stale because it has been open for too long.
Projects
None yet
Development

No branches or pull requests

3 participants