Skip to content

Add ldd --version check#10

Merged
GeckoEidechse merged 32 commits intoR2NorthstarTools:mainfrom
TH3-S4LM0N:main
Oct 18, 2022
Merged

Add ldd --version check#10
GeckoEidechse merged 32 commits intoR2NorthstarTools:mainfrom
TH3-S4LM0N:main

Conversation

@TH3-S4LM0N
Copy link
Copy Markdown
Contributor

hahhah finally the button works

Added a function in linux.rs to check ldd --version >= 2.33, but rn its inside of a general linux_checks function. Returns a bool so that anything about checking linux compatibility in the future can just be 1 button.

@GeckoEidechse GeckoEidechse self-assigned this Oct 6, 2022
Copy link
Copy Markdown
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Thanks for the PR <3

Noticed a few things and annotated them accordingly. Need to test compile on Windows still but if it does (and the annotated things are resolved) should be good to merge ^^

I assume it worked on your machine?

Copy link
Copy Markdown
Collaborator

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Lacks some comments and features some typos

@GeckoEidechse GeckoEidechse mentioned this pull request Oct 7, 2022
100 tasks
@TH3-S4LM0N
Copy link
Copy Markdown
Contributor Author

Ok I think all review requests have been satisfied. It built and ran on both linux and windows on my machine, since it's still in the dev view it's not implemented. Next I'll prolly do nsproton and work this all into that.

Copy link
Copy Markdown
Collaborator

@Alystrasz Alystrasz left a comment

Choose a reason for hiding this comment

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

Typos, typos everywhere

Copy link
Copy Markdown
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Doesn't break Windows and I assume Linux was tested, so fine to merge by me ^^

feat: Show own version number in settings view (#11)
Copy link
Copy Markdown
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

So it works on Linux now without crash :D

But Windows fails to compile now atm ._.

serde_json = "1.0"
serde = { version = "1.0", features = ["derive"] }
tauri = { version = "1.1.1", features = ["api-all", "updater"] }
tauri = { version = "1.1.1", features = ["api-all"] }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh keep forgetting this

Comment on lines -1773 to -1778
[[package]]
name = "minisign-verify"
version = "0.2.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "933dca44d65cdd53b355d0b73d380a2ff5da71f87f036053188bf1eab6a19881"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

btw, why is Cargo.lock not in the gitignore?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It allows developers to have exact same versions of dependencies

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's the same as NPM and package-lock.json. Locks dependency to certain version for reproducible builds.

Comment on lines +123 to +129
fn linux_checks() -> bool {
if get_host_os() == "windows" {
false
} else {
linux_checks_librs()
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should probably be changed to returning a Result<T,E> type in the future but fine as is for now for me ^^

@GeckoEidechse
Copy link
Copy Markdown
Member

Tested working on Linux, will merge if CI passes.

@GeckoEidechse GeckoEidechse merged commit c4ce52b into R2NorthstarTools:main Oct 18, 2022
@GeckoEidechse GeckoEidechse removed their assignment Sep 24, 2024
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.

3 participants