Skip to content

Fix ResourceImporter* methods not found in Release mode; add Release CI #537

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

Merged
merged 4 commits into from
Dec 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/composite/godot-itest/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ inputs:
default: ''
description: "If provided, acts as an argument for '--target', and results in output files written to ./target/{target}"

rust-cache-key:
required: false
default: ''
description: "Extra key to resolve Rust cache"

with-llvm:
required: false
default: ''
Expand Down Expand Up @@ -82,6 +87,7 @@ runs:
with:
rust: ${{ inputs.rust-toolchain }}
with-llvm: ${{ inputs.with-llvm }}
cache-key: ${{ inputs.rust-cache-key }}

- name: "Patch prebuilt version ({{ inputs.godot-prebuilt-patch }})"
if: inputs.godot-prebuilt-patch != ''
Expand Down
13 changes: 11 additions & 2 deletions .github/workflows/full-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -230,18 +230,26 @@ jobs:
godot-binary: godot.linuxbsd.editor.dev.x86_64
rust-extra-args: --features godot/custom-godot

- name: linux-double
# Full codegen could also be moved to another job.
- name: linux-double-full
os: ubuntu-20.04
artifact-name: linux-double-nightly
godot-binary: godot.linuxbsd.editor.dev.double.x86_64
rust-extra-args: --features godot/custom-godot,godot/double-precision
rust-extra-args: --features godot/custom-godot,godot/double-precision,godot/codegen-full

- name: linux-features
os: ubuntu-20.04
artifact-name: linux-nightly
godot-binary: godot.linuxbsd.editor.dev.x86_64
rust-extra-args: --features godot/custom-godot,godot/experimental-threads,godot/serde

- name: linux-release
os: ubuntu-20.04
artifact-name: linux-release-nightly
godot-binary: godot.linuxbsd.template_release.x86_64
rust-extra-args: --release
rust-cache-key: release

# TODO merge with other jobs
- name: linux-lazy-fptrs
os: ubuntu-20.04
Expand Down Expand Up @@ -321,6 +329,7 @@ jobs:
rust-toolchain: ${{ matrix.rust-toolchain || 'stable' }}
rust-env-rustflags: ${{ matrix.rust-env-rustflags }}
rust-target: ${{ matrix.rust-target }}
rust-cache-key: ${{ matrix.rust-cache-key }}
with-llvm: ${{ contains(matrix.name, 'macos') && contains(matrix.rust-extra-args, 'custom-godot') }}
godot-check-header: ${{ matrix.godot-check-header }}

Expand Down
4 changes: 2 additions & 2 deletions godot-codegen/src/central_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1021,7 +1021,7 @@ fn populate_class_methods(
let mut method_inits = vec![];

for method in option_as_slice(&class.methods) {
if special_cases::is_deleted(class_ty, method, ctx) {
if special_cases::is_class_method_deleted(class_ty, method, ctx) {
continue;
}

Expand Down Expand Up @@ -1084,7 +1084,7 @@ fn populate_builtin_methods(

for method in option_as_slice(&builtin_class.methods) {
let builtin_ty = TyName::from_godot(&builtin_class.name);
if special_cases::is_builtin_deleted(&builtin_ty, method) {
if special_cases::is_builtin_method_deleted(&builtin_ty, method) {
continue;
}

Expand Down
19 changes: 11 additions & 8 deletions godot-codegen/src/class_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1140,18 +1140,18 @@ fn make_class_method_definition(
get_method_table: &Ident,
ctx: &mut Context,
) -> FnDefinition {
if special_cases::is_deleted(class_name, method, ctx) {
if special_cases::is_class_method_deleted(class_name, method, ctx) {
return FnDefinition::none();
}

let class_name_str = &class_name.godot_ty;
let godot_method_name = &method.name;
let rust_method_name = special_cases::maybe_renamed(class_name, godot_method_name);
let rust_method_name = special_cases::maybe_rename_class_method(class_name, godot_method_name);

// Override const-qualification for known special cases (FileAccess::get_16, StreamPeer::get_u16, etc.).
/* TODO enable this once JSON/domain models are separated. Remove #[allow] above.
let mut override_is_const = None;
if let Some(override_const) = special_cases::is_method_const(class_name, &method) {
if let Some(override_const) = special_cases::is_class_method_const(class_name, &method) {
override_is_const = Some(override_const);
}

Expand Down Expand Up @@ -1226,7 +1226,7 @@ fn make_class_method_definition(
&FnSignature {
function_name: rust_method_name,
surrounding_class: Some(class_name),
is_private: special_cases::is_private(class_name, godot_method_name),
is_private: special_cases::is_method_private(class_name, godot_method_name),
is_virtual: false,
is_vararg: method.is_vararg,
qualifier: FnQualifier::for_method(method.is_const, method.is_static),
Expand All @@ -1247,7 +1247,7 @@ fn make_builtin_method_definition(
inner_class_name: &TyName,
ctx: &mut Context,
) -> FnDefinition {
if special_cases::is_builtin_deleted(builtin_name, method) {
if special_cases::is_builtin_method_deleted(builtin_name, method) {
return FnDefinition::none();
}

Expand Down Expand Up @@ -1308,7 +1308,7 @@ fn make_builtin_method_definition(
&FnSignature {
function_name: method_name_str,
surrounding_class: Some(inner_class_name),
is_private: special_cases::is_private(builtin_name, &method.name),
is_private: special_cases::is_method_private(builtin_name, &method.name),
is_virtual: false,
is_vararg: method.is_vararg,
qualifier: FnQualifier::for_method(method.is_const, method.is_static),
Expand Down Expand Up @@ -1946,7 +1946,7 @@ fn make_all_virtual_methods(
all_virtuals
.into_iter()
.filter_map(|method| {
if codegen_special_cases::is_method_excluded(&method, true, ctx) {
if codegen_special_cases::is_class_method_excluded(&method, true, ctx) {
None
} else {
Some(make_virtual_method(&method, ctx))
Expand Down Expand Up @@ -1994,5 +1994,8 @@ fn function_uses_pointers(sig: &FnSignature) -> bool {

fn function_uses_default_params(sig: &FnSignature) -> bool {
sig.params.iter().any(|arg| arg.default_value.is_some())
&& !special_cases::is_excluded_from_default_params(sig.surrounding_class, sig.function_name)
&& !special_cases::is_method_excluded_from_default_params(
sig.surrounding_class,
sig.function_name,
)
}
2 changes: 1 addition & 1 deletion godot-codegen/src/codegen_special_cases.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ fn is_type_excluded(ty: &str, ctx: &mut Context) -> bool {
is_rust_type_excluded(&util::to_rust_type(ty, None, ctx))
}

pub(crate) fn is_method_excluded(
pub(crate) fn is_class_method_excluded(
method: &ClassMethod,
is_virtual_impl: bool,
ctx: &mut Context,
Expand Down
4 changes: 2 additions & 2 deletions godot-codegen/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ impl<'a> Context<'a> {
}

for method in methods.iter() {
if special_cases::is_deleted(class_name, method, ctx) {
if special_cases::is_class_method_deleted(class_name, method, ctx) {
continue;
}

Expand All @@ -190,7 +190,7 @@ impl<'a> Context<'a> {
}

for method in methods.iter() {
if special_cases::is_builtin_deleted(&builtin_ty, method) {
if special_cases::is_builtin_method_deleted(&builtin_ty, method) {
continue;
}

Expand Down
28 changes: 18 additions & 10 deletions godot-codegen/src/special_cases.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@

// Lists all cases in the Godot class API, where deviations are considered appropriate (e.g. for safety).

// Naming:
// * Class methods: is_class_method_*
// * Builtin methods: is_builtin_method_*
// * Class or builtin methods: is_method_*

// Open design decisions:
// * Should Godot types like Node3D have all the "obj level" methods like to_string(), get_instance_id(), etc; or should those
// be reserved for the Gd<T> pointer? The latter seems like a limitation. User objects also have to_string() (but not get_instance_id())
Expand All @@ -23,8 +28,8 @@ use crate::Context;
use crate::{codegen_special_cases, TyName};

#[rustfmt::skip]
pub(crate) fn is_deleted(class_name: &TyName, method: &ClassMethod, ctx: &mut Context) -> bool {
if codegen_special_cases::is_method_excluded(method, false, ctx){
pub(crate) fn is_class_method_deleted(class_name: &TyName, method: &ClassMethod, ctx: &mut Context) -> bool {
if codegen_special_cases::is_class_method_excluded(method, false, ctx){
return true;
}

Expand Down Expand Up @@ -147,12 +152,12 @@ fn is_class_experimental(class_name: &TyName) -> bool {
pub(crate) fn is_named_accessor_in_table(class_or_builtin_ty: &TyName, godot_method_name: &str) -> bool {
// Generated methods made private are typically needed internally and exposed with a different API,
// so make them accessible.
is_private(class_or_builtin_ty, godot_method_name)
is_method_private(class_or_builtin_ty, godot_method_name)
}

/// Whether a class or builtin method should be hidden from the public API.
#[rustfmt::skip]
pub(crate) fn is_private(class_or_builtin_ty: &TyName, godot_method_name: &str) -> bool {
pub(crate) fn is_method_private(class_or_builtin_ty: &TyName, godot_method_name: &str) -> bool {
match (class_or_builtin_ty.godot_ty.as_str(), godot_method_name) {
// Already covered by manual APIs
| ("Object", "to_string")
Expand All @@ -166,7 +171,7 @@ pub(crate) fn is_private(class_or_builtin_ty: &TyName, godot_method_name: &str)
}

#[rustfmt::skip]
pub(crate) fn is_excluded_from_default_params(class_name: Option<&TyName>, godot_method_name: &str) -> bool {
pub(crate) fn is_method_excluded_from_default_params(class_name: Option<&TyName>, godot_method_name: &str) -> bool {
// None if global/utilities function
let class_name = class_name.map_or("", |ty| ty.godot_ty.as_str());

Expand All @@ -184,7 +189,7 @@ pub(crate) fn is_excluded_from_default_params(class_name: Option<&TyName>, godot
/// since it looks like a getter.
#[rustfmt::skip]
#[cfg(FALSE)] // TODO enable this once JSON/domain models are separated.
pub(crate) fn is_method_const(class_name: &TyName, godot_method: &ClassMethod) -> Option<bool> {
pub(crate) fn is_class_method_const(class_name: &TyName, godot_method: &ClassMethod) -> Option<bool> {
match (class_name.godot_ty.as_str(), godot_method.name.as_str()) {
// Changed to mut.
| ("FileAccess", "get_16")
Expand All @@ -211,23 +216,26 @@ pub(crate) fn is_method_const(class_name: &TyName, godot_method: &ClassMethod) -
}

/// True if builtin method is excluded. Does NOT check for type exclusion; use [`is_builtin_type_deleted`] for that.
pub(crate) fn is_builtin_deleted(_class_name: &TyName, method: &BuiltinClassMethod) -> bool {
pub(crate) fn is_builtin_method_deleted(_class_name: &TyName, method: &BuiltinClassMethod) -> bool {
// Currently only deleted if codegen.
codegen_special_cases::is_builtin_method_excluded(method)
}

/// True if builtin type is excluded (`NIL` or scalars)
pub(crate) fn is_builtin_type_deleted(class_name: &TyName) -> bool {
let name = class_name.godot_ty.as_str();
name == "Nil" || is_builtin_scalar(name)
name == "Nil" || is_builtin_type_scalar(name)
}

/// True if `int`, `float`, `bool`, ...
pub(crate) fn is_builtin_scalar(name: &str) -> bool {
pub(crate) fn is_builtin_type_scalar(name: &str) -> bool {
name.chars().next().unwrap().is_ascii_lowercase()
}

pub(crate) fn maybe_renamed<'m>(class_name: &TyName, godot_method_name: &'m str) -> &'m str {
pub(crate) fn maybe_rename_class_method<'m>(
class_name: &TyName,
godot_method_name: &'m str,
) -> &'m str {
match (class_name.godot_ty.as_str(), godot_method_name) {
// GDScript, GDScriptNativeClass, possibly more in the future
(_, "new") => "instantiate",
Expand Down
17 changes: 13 additions & 4 deletions godot-codegen/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
use crate::api_parser::{
BuiltinClassMethod, Class, ClassConstant, ClassMethod, ConstValue, Enum, UtilityFunction,
};
use crate::special_cases::is_builtin_scalar;
use crate::special_cases::is_builtin_type_scalar;
use crate::{Context, GodotTy, ModName, RustTy, TyName};

use proc_macro2::{Ident, Literal, TokenStream};
Expand Down Expand Up @@ -198,12 +198,21 @@ pub fn make_sname_ptr(identifier: &str) -> TokenStream {
}

pub fn get_api_level(class: &Class) -> ClassCodegenLevel {
// Work around wrong classification in https://github.com/godotengine/godot/issues/86206.
fn override_editor(class_name: &str) -> bool {
cfg!(before_api = "4.3")
&& matches!(
class_name,
"ResourceImporterOggVorbis" | "ResourceImporterMP3"
)
}

if class.name.ends_with("Server") {
ClassCodegenLevel::Servers
} else if class.api_type == "editor" || override_editor(&class.name) {
ClassCodegenLevel::Editor
} else if class.api_type == "core" {
ClassCodegenLevel::Scene
} else if class.api_type == "editor" {
ClassCodegenLevel::Editor
} else {
panic!(
"class {} has unknown API type {}",
Expand Down Expand Up @@ -640,7 +649,7 @@ fn to_rust_type_uncached(full_ty: &GodotTy, ctx: &mut Context) -> RustTy {

/// Transforms a Godot class/builtin/enum IDENT (without `::` or other syntax) to a Rust one
fn rustify_ty(ty: &str) -> Ident {
if is_builtin_scalar(ty) {
if is_builtin_type_scalar(ty) {
ident(ty)
} else {
TyName::from_godot(ty).rust_ty
Expand Down
6 changes: 6 additions & 0 deletions itest/godot/ManualFfiTests.gd
Original file line number Diff line number Diff line change
Expand Up @@ -253,13 +253,19 @@ func test_custom_property():
assert_eq(d.b, 33)

func test_custom_property_wrong_values_1():
if runs_release():
return

var has_property: HasCustomProperty = HasCustomProperty.new()
disable_error_messages()
has_property.some_c_style_enum = 10 # Should fail.
enable_error_messages()
assert_fail("HasCustomProperty.some_c_style_enum should only accept integers in the range `(0 ..= 2)`")

func test_custom_property_wrong_values_2():
if runs_release():
return

var has_property: HasCustomProperty = HasCustomProperty.new()
disable_error_messages()
has_property.not_exportable = {"a": "hello", "b": Callable()} # Should fail.
Expand Down
5 changes: 5 additions & 0 deletions itest/godot/TestSuite.gd
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,8 @@ func assert_fail(message: String = "") -> bool:
print_error("Test execution should have failed")

return false

## Some tests are disabled, as they rely on Godot checks which are only available in Debug builds.
## See https://github.com/godotengine/godot/issues/86264.
static func runs_release() -> bool:
return !OS.is_debug_build()
24 changes: 13 additions & 11 deletions itest/rust/src/builtin_tests/containers/variant_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use godot::obj::InstanceId;
use godot::sys::GodotFfi;

use crate::common::roundtrip;
use crate::framework::{expect_panic, itest};
use crate::framework::{expect_panic, itest, runs_release};

const TEST_BASIS: Basis = Basis::from_rows(
Vector3::new(1.0, 2.0, 3.0),
Expand Down Expand Up @@ -146,16 +146,18 @@ fn variant_call() {
let result = vector.to_variant().call("dot", &[vector_rhs.to_variant()]);
assert_eq!(result, 2.0.to_variant());

// Error cases
expect_panic("Variant::call on non-existent method", || {
variant.call("gut_position", &[]);
});
expect_panic("Variant::call with bad signature", || {
variant.call("set_position", &[]);
});
expect_panic("Variant::call with non-object variant (int)", || {
Variant::from(77).call("to_string", &[]);
});
// Dynamic checks are only available in Debug builds.
if !runs_release() {
expect_panic("Variant::call on non-existent method", || {
variant.call("gut_position", &[]);
});
expect_panic("Variant::call with bad signature", || {
variant.call("set_position", &[]);
});
expect_panic("Variant::call with non-object variant (int)", || {
Variant::from(77).call("to_string", &[]);
});
}

node2d.free();
}
Expand Down
8 changes: 7 additions & 1 deletion itest/rust/src/framework/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/

use godot::engine::{Engine, Node};
use godot::engine::{Engine, Node, Os};
use godot::obj::Gd;
use godot::sys;
use std::collections::HashSet;
Expand Down Expand Up @@ -128,3 +128,9 @@ pub fn suppress_godot_print(mut f: impl FnMut()) {
f();
Engine::singleton().set_print_error_messages(true);
}

/// Some tests are disabled, as they rely on Godot checks which are only available in Debug builds.
/// See https://github.com/godotengine/godot/issues/86264.
pub fn runs_release() -> bool {
!Os::singleton().is_debug_build()
}