-
Notifications
You must be signed in to change notification settings - Fork 59
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 Dockerhub publishing step to the release GitHub Action workflow #740
Changes from 6 commits
2b2cd9c
c5dac54
c5a69f3
a9d9ff1
6de40bc
61ee39b
e4d8f2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,7 +51,6 @@ pub mod metta_interface_mod { | |
} | ||
|
||
pub fn confirm_hyperonpy_version(req_str: &str) -> Result<(), String> { | ||
|
||
let req = parse_version_specifiers(req_str).unwrap(); | ||
let version_string = get_hyperonpy_version()?; | ||
//NOTE: Version parsing errors will be encountered by users building hyperonpy from source with an abnormal configuration | ||
|
@@ -78,7 +77,8 @@ pub mod metta_interface_mod { | |
|
||
match || -> Result<_, String> { | ||
//Confirm the hyperonpy version is compatible | ||
confirm_hyperonpy_version(">=0.1.0, <0.2.0")?; | ||
let req_str = Self::required_hyperon_version(); | ||
confirm_hyperonpy_version(&req_str)?; | ||
|
||
//Initialize the Hyperon environment | ||
let new_shim = MettaShim::init_common_env(working_dir, include_paths)?; | ||
|
@@ -93,6 +93,11 @@ pub mod metta_interface_mod { | |
} | ||
} | ||
|
||
fn required_hyperon_version() -> String { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I admit my hard-coded requirement was a bad solution and I am glad you are improving it. I agree with what I think you're trying to do by using the repl's However, I don't like this approach of creating a VersionReq for HyperonPy by assuming it'll be within 1 minor version of the repl version. One thought is to use a requirement string created from the actual hyperonpy version in CMake. This makes releasing easy, but it becomes annoying to use cargo directly to build the repl. Another thought is to use a custom metadata key in the Cargo.toml. It's still two keys to update but they're in one place. For example:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. UPDATE: After trying to implement the custom Cargo.toml metadata solution, it seems like a clumsy solution. It appears to require parsing the Cargo.toml file ourselves. (At lease if the way docs.rs does it is the recommended way. https://github.com/rust-lang/docs.rs/blob/7ad8c626bd63a95dc483143274ff6f2410a34af9/crates/metadata/lib.rs#L103) My thinking is that, if a "correct" solution is clumsy and probably not necessary, we should go with a "mostly-works" solution that is as simple as possible. The purpose of the check is to prevent version incompatibilities from showing up as mysterious bugs that we waste time chasing. So if we plan to keep the components within a release in version lock-step, then I don't think we need the semver range. So how about something like this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree that if we split repo on different components: corelib, python lib, repl, then we need to specify repl dependency manually. And keeping it in metadata looks like a proper way of doing this. I didn't think about it on the first place because Python package version right now is formed from the GitHub tag and its I agree that we can just require this versions to be equal while we have them in a same repo. This implementation should cover most of the cases. In practice dev Python package version always contains local version like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Applied your suggestion in 61ee39b |
||
const PACKAGE_VERSION: &str = env!("CARGO_PKG_VERSION"); | ||
format!("=={PACKAGE_VERSION}") | ||
} | ||
|
||
pub fn init_common_env(working_dir: PathBuf, include_paths: Vec<PathBuf>) -> Result<MettaShim, String> { | ||
match Python::with_gil(|py| -> PyResult<(Py<PyModule>, Py<PyAny>)> { | ||
let py_mod = PyModule::from_code(py, Self::PY_CODE, "", "")?; | ||
|
@@ -470,4 +475,4 @@ pub fn strip_quotes(src: &str) -> &str { | |
} | ||
} | ||
src | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Python module versions aren't semver, they're pep440. It turns out there is enough overlap that it's possible to parse one as the other in the common cases, but I don't think it's right to rely on this coincidence.