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

add riscv build support #593

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add riscv build support #593

wants to merge 2 commits into from

Conversation

dista
Copy link

@dista dista commented Dec 13, 2023

old ring(tls related) do not support RISC-V, so update it to latest version.
also it will impact sqlx(upgrade from 0.5 to 0.7), 0.7 version sqlx remove Excutor trait in Transaction, so code need to be
modified for that.

@mental32
Copy link
Collaborator

Just so I understand: to enable riscv support this PR is essentially just updating the versions of some dependencies?

@dista
Copy link
Author

dista commented Dec 31, 2023

Just so I understand: to enable riscv support this PR is essentially just updating the versions of some dependencies?

yes, I have a risc-v board, and after updating deps, it works fine.
because old ring does not support risc-v(briansmith/ring#1182), update to latest version will make dim
work on risc-v

@@ -248,7 +248,7 @@ impl Episode {
season_number,
season_number
)
.fetch_one(&mut *conn)
.fetch_one(conn.as_mut())
Copy link
Member

Choose a reason for hiding this comment

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

I feel like these changes are mostly pedantic.

Copy link
Author

Choose a reason for hiding this comment

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

I will try to see if it can be done in another way

Copy link
Author

@dista dista Jan 2, 2024

Choose a reason for hiding this comment

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

Because Excutor trait is no longer impl for Transaction.

Some option:

  1. I see another pull request Bump sqlx to 0.7.3 #597 which use extra level of deref like this
-            .fetch_all(&mut *conn)
+           .fetch_all(&mut **conn)
  1. or we implement excutor back for Transaction.

which way do you prefer

Choose a reason for hiding this comment

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

The reason there isn't a implementation of Executor for Transaction in sqlx now is that it couldn't exist. The changelog of sqlx explicitly said that dereferencing the Transaction is the way to do it now, so not sure how option 2 is going to work?

Copy link
Author

Choose a reason for hiding this comment

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

The reason there isn't a implementation of Executor for Transaction in sqlx now is that it couldn't exist. The changelog of sqlx explicitly said that dereferencing the Transaction is the way to do it now, so not sure how option 2 is going to work?

ha, seems so.

@donmor
Copy link

donmor commented Mar 6, 2024

I built this fork and dim.db get locked after creating admin account
2024-03-06T01:13:39.402942Z ERROR request{method=POST uri=/api/v1/library version=HTTP/1.1}: dim_web::routes::library: Error committing transaction err=Database(SqliteError { code: 5, message: "database is locked" })

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.

6 participants