Skip to content

[FEAT] Install WASM file from numscript-wasm release (CU #86b3a5va4)#4

Merged
fermuch merged 43 commits intomainfrom
feat/download-wasm-file-from-release#86b3a5va4
Jan 30, 2025
Merged

[FEAT] Install WASM file from numscript-wasm release (CU #86b3a5va4)#4
fermuch merged 43 commits intomainfrom
feat/download-wasm-file-from-release#86b3a5va4

Conversation

@Vinocis
Copy link
Collaborator

@Vinocis Vinocis commented Jan 3, 2025

PR Type

enhancement, tests, bug_fix


Description

  • Introduced Numscriptex.CompilationSettings module for WASM binary management.

  • Added tests for WASM binary installation and validation.

  • Improved error handling and logging for WASM operations.

  • Fixed typos and improved code formatting.


Changes walkthrough 📝

Relevant files
Enhancement
numscriptex.ex
Integrate WASM binary management and logging                         

lib/numscriptex.ex

  • Added Numscriptex.CompilationSettings for WASM binary management.
  • Improved logging with Logger.
  • Changed binary reading to use @binary_path.
  • +12/-8   
    compilation_settings.ex
    Add WASM binary management module                                               

    lib/numscriptex/compilation_settings.ex

  • New module for managing WASM binary installation.
  • Includes checksum validation and error handling.
  • Provides detailed logging for operations.
  • +153/-0 
    Tests
    compilation_settings_test.exs
    Add tests for WASM binary management                                         

    test/numscriptex/compilation_settings_test.exs

  • Added tests for WASM binary installation and validation.
  • Utilized ExUnit.CaptureLog for log testing.
  • Tested scenarios for binary existence and validity.
  • +81/-0   
    Bug fix
    numscriptex_test.exs
    Fix error assertions in tests                                                       

    test/numscriptex_test.exs

  • Corrected assertions from error.reason to error.details.
  • Improved test accuracy for error messages.
  • +3/-3     
    Additional files
    numscript.wasm [link]   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @Vinocis Vinocis self-assigned this Jan 3, 2025
    @Vinocis Vinocis marked this pull request as draft January 3, 2025 18:16
    @Vinocis Vinocis requested a review from fermuch January 3, 2025 18:16
    @github-actions
    Copy link

    github-actions bot commented Jan 3, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 1a426d8)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    WASM Binary Management

    The introduction of Numscriptex.CompilationSettings for managing WASM binaries requires careful review to ensure that the binary installation and validation processes are robust and error-free.

    require Numscriptex.CompilationSettings
    require Logger
    
    @type check_log() :: CheckLog.t()
    
    @type check_result() :: %{
            required(:script) => binary(),
            optional(:hints) => list(check_log()),
            optional(:infos) => list(check_log()),
            optional(:warnings) => list(check_log())
          }
    
    @type run_result() :: %{
            required(:balances) => balances(),
            required(:postings) => postings(),
            required(:accountsMeta) => map(),
            required(:txMeta) => map()
          }
    
    @type balances() :: %{
            required(:initial_balance) => integer(),
            required(:final_balance) => integer(),
            required(:asset) => binary(),
            required(:account) => binary()
          }
    
    @type postings() :: %{
            required(:destination) => binary(),
            required(:source) => binary(),
            required(:asset) => binary(),
            required(:amount) => integer()
          }
    
    @type errors() :: %{
            required(:reason) => list(check_log()) | any(),
            optional(:details) => any()
          }
    
    @binary_path :numscriptex
                 |> :code.priv_dir()
                 |> Path.join("numscript.wasm")
    
    Numscriptex.CompilationSettings.ensure_wasm_binary_is_installed_and_valid()
    Error Handling

    The error handling in the download_wasm_file and remote_checksums functions should be reviewed to ensure that all potential failure cases are adequately addressed and logged.

    defp download_wasm_file do
      quote do
        Logger.info("Downloading Numscript-WASM binary.")
    
        request =
          :httpc.request(
            :get,
            {unquote(@numscript_wasm_url), []},
            [],
            stream: unquote(@binary_path)
          )
    
        case request do
          {:ok, :saved_to_file} ->
            Logger.info("Numscript-WASM binary downloaded.")
    
            :ok
    
          {:ok, {{_, status_code, status_message}, _, _}} when status_code not in 200..299 ->
            Logger.error(
              "Download request failed with status code #{status_code} - #{status_message}."
            )
    
            raise CompileError, file: "./lib/numsriptex/compilation_settings.ex", line: 98
    
          {:error, reason} ->
            Logger.error(
              "Failed to download Numscript-WASM binary. Reason: #{inspect(reason)}. Binary path: #{unquote(@binary_path)}"
            )
    
            raise CompileError, file: "./lib/numsriptex/compilation_settings.ex", line: 105
        end
      end
    end
    
    defp remote_checksums do
      quote do
        Logger.info("Getting remote checksums.")
    
        case :httpc.request(:get, {unquote(@numscript_checksums_url), []}, [], []) do
          {:ok, {{_, status_code, status_message}, _, _}} when status_code not in 200..299 ->
            Logger.error(
              "Download request failed with status code #{status_code} - #{status_message}."
            )
    
            raise CompileError, file: "./lib/numsriptex/compilation_settings.ex", line: 118
    
          {:ok, {{_protocol, _status_code, _status_message}, _header, body}} ->
            Logger.info("Numscript-WASM checksums downloaded.")
    
            [checksums, _file_name] =
              body
              |> to_string()
              |> String.split()
    
            {:ok, String.trim(checksums)}
    
          {:error, reason} ->
            Logger.error(
              "Failed to download Numscript-WASM checksums from release assets. Reason: #{reason}."
            )
    
            raise CompileError, file: "./lib/numsriptex/compilation_settings.ex", line: 135

    @github-actions
    Copy link

    github-actions bot commented Jan 3, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 1a426d8
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Correct file path in errors

    Ensure that all raise CompileError calls include the correct file path and line
    number to aid in debugging.

    lib/numscriptex/compilation_settings.ex [49]

    -raise CompileError, file: "./lib/numsriptex/compilation_settings.ex", line: 49
    +raise CompileError, file: "./lib/numscriptex/compilation_settings.ex", line: 49
    Suggestion importance[1-10]: 9

    Why: Correcting the file path in error messages is crucial for accurate debugging. This suggestion addresses a clear mistake in the code, making it highly impactful.

    9
    Possible issue
    Handle directory creation errors

    Add error handling for File.mkdir_p/1 to ensure the directory creation is successful
    before proceeding with binary operations.

    lib/numscriptex/compilation_settings.ex [16]

    -File.mkdir_p(:code.priv_dir(:numscriptex))
    +case File.mkdir_p(:code.priv_dir(:numscriptex)) do
    +  :ok -> # proceed
    +  {:error, reason} -> Logger.error("Failed to create directory: #{inspect(reason)}")
    +end
    Suggestion importance[1-10]: 8

    Why: Adding error handling for directory creation is a practical improvement that can prevent subsequent operations from failing silently, enhancing the robustness of the code.

    8
    Ensure WASM binary readiness

    Ensure that the ensure_wasm_binary_is_installed_and_valid function is called at the
    appropriate time during the application lifecycle to avoid runtime errors if the
    WASM binary is not ready.

    lib/numscriptex.ex [56]

    +# Ensure this function is called during application startup or in a compile-time hook
     Numscriptex.CompilationSettings.ensure_wasm_binary_is_installed_and_valid()
    Suggestion importance[1-10]: 7

    Why: The suggestion to ensure the WASM binary is ready at the appropriate time during the application lifecycle is valid and can prevent runtime errors. However, it is more of a reminder than a direct code change, which slightly reduces its impact.

    7

    Previous suggestions

    Suggestions up to commit 1f5bd03
    CategorySuggestion                                                                                                                                    Score
    General
    Remove unnecessary debug output from the code

    Remove the debug statement IO.puts("AAAA") as it appears to be left over from
    debugging and is not necessary for the functionality.

    lib/numscriptex/compilation_settings.ex [36]

    -IO.puts("AAAA")
    +# Removed unnecessary debug statement
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies and removes a debug statement that is unnecessary for the functionality, improving code cleanliness and professionalism.

    9
    Possible issue
    Ensure the target directory exists before extracting files to avoid errors

    Ensure that the directory specified by :code.priv_dir() exists before attempting to
    extract files into it to prevent runtime errors.

    lib/numscriptex/compilation_settings.ex [23]

    +File.mkdir_p!(:code.priv_dir())
     :erl_tar.extract(unquote(@download_path), [{:cwd, "priv"}, :compressed])
    Suggestion importance[1-10]: 8

    Why: This suggestion addresses a potential runtime error by ensuring the target directory exists before file extraction, which enhances the robustness of the code.

    8

    @Vinocis Vinocis marked this pull request as ready for review January 28, 2025 13:33
    @github-actions
    Copy link

    Persistent review updated to latest commit 1a426d8

    Copy link
    Contributor

    @fermuch fermuch left a comment

    Choose a reason for hiding this comment

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

    Falta adicionar uma matrix de testes para rodar a test suite tanto em Windows como OSX além de Linux.

    @Vinocis Vinocis requested a review from fermuch January 29, 2025 20:53
    @Vinocis Vinocis requested a review from fermuch January 30, 2025 15:03
    Copy link
    Contributor

    @fermuch fermuch left a comment

    Choose a reason for hiding this comment

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

    LGTM!

    @fermuch fermuch merged commit c77cedd into main Jan 30, 2025
    4 checks passed
    @fermuch fermuch deleted the feat/download-wasm-file-from-release#86b3a5va4 branch January 30, 2025 17:18
    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.

    2 participants