From e755f1a0b5b588f51b26e8ac9eddd69fba044567 Mon Sep 17 00:00:00 2001 From: Fabian Dill Date: Wed, 12 Jun 2024 02:14:30 +0200 Subject: [PATCH 1/7] SC2: don't close all SC2 instances when one quits (#3507) --- worlds/_sc2common/bot/sc2process.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/worlds/_sc2common/bot/sc2process.py b/worlds/_sc2common/bot/sc2process.py index e36632165979..f74ed9c18f9f 100644 --- a/worlds/_sc2common/bot/sc2process.py +++ b/worlds/_sc2common/bot/sc2process.py @@ -28,6 +28,11 @@ def add(cls, value): logger.debug("kill_switch: Add switch") cls._to_kill.append(value) + @classmethod + def kill(cls, value): + logger.info(f"kill_switch: Process cleanup for 1 process") + value._clean(verbose=False) + @classmethod def kill_all(cls): logger.info(f"kill_switch: Process cleanup for {len(cls._to_kill)} processes") @@ -116,7 +121,7 @@ def signal_handler(*_args): async def __aexit__(self, *args): logger.exception("async exit") await self._close_connection() - kill_switch.kill_all() + kill_switch.kill(self) signal.signal(signal.SIGINT, signal.SIG_DFL) @property From 7299891bdf86d5134692ae55fac92ad32ba17059 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Tue, 11 Jun 2024 18:22:14 -0700 Subject: [PATCH 2/7] Allow worlds to add options to prebuilt groups (#3509) Previously, this crashed because `typing.NamedTuple` fields such as `group.name` aren't assignable. Now it will only fail for group names that are actually incorrectly cased, and will fail with a better error message. --- worlds/AutoWorld.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/worlds/AutoWorld.py b/worlds/AutoWorld.py index 6e17f023f6fb..bed375cf080a 100644 --- a/worlds/AutoWorld.py +++ b/worlds/AutoWorld.py @@ -123,8 +123,8 @@ def __new__(mcs, name: str, bases: Tuple[type, ...], dct: Dict[str, Any]) -> Web assert group.options, "A custom defined Option Group must contain at least one Option." # catch incorrectly titled versions of the prebuilt groups so they don't create extra groups title_name = group.name.title() - if title_name in prebuilt_options: - group.name = title_name + assert title_name not in prebuilt_options or title_name == group.name, \ + f"Prebuilt group name \"{group.name}\" must be \"{title_name}\"" if group.name == "Item & Location Options": assert not any(option in item_and_loc_options for option in group.options), \ From b9e454ab4ec7903a66e71324805f93b0332bc286 Mon Sep 17 00:00:00 2001 From: Silvris <58583688+Silvris@users.noreply.github.com> Date: Tue, 11 Jun 2024 20:23:46 -0500 Subject: [PATCH 3/7] TS: add indirect connections (#3490) --- worlds/timespinner/Regions.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/worlds/timespinner/Regions.py b/worlds/timespinner/Regions.py index 4f53f75eff7a..757a41c38821 100644 --- a/worlds/timespinner/Regions.py +++ b/worlds/timespinner/Regions.py @@ -70,7 +70,7 @@ def create_regions_and_locations(world: MultiWorld, player: int, precalculated_w logic = TimespinnerLogic(world, player, precalculated_weights) connect(world, player, 'Lake desolation', 'Lower lake desolation', lambda state: flooded.flood_lake_desolation or logic.has_timestop(state) or state.has('Talaria Attachment', player)) - connect(world, player, 'Lake desolation', 'Upper lake desolation', lambda state: logic.has_fire(state) and state.can_reach('Upper Lake Serene', 'Region', player)) + connect(world, player, 'Lake desolation', 'Upper lake desolation', lambda state: logic.has_fire(state) and state.can_reach('Upper Lake Serene', 'Region', player), "Upper Lake Serene") connect(world, player, 'Lake desolation', 'Skeleton Shaft', lambda state: flooded.flood_lake_desolation or logic.has_doublejump(state)) connect(world, player, 'Lake desolation', 'Space time continuum', logic.has_teleport) connect(world, player, 'Upper lake desolation', 'Lake desolation') @@ -80,7 +80,7 @@ def create_regions_and_locations(world: MultiWorld, player: int, precalculated_w connect(world, player, 'Eastern lake desolation', 'Space time continuum', logic.has_teleport) connect(world, player, 'Eastern lake desolation', 'Library') connect(world, player, 'Eastern lake desolation', 'Lower lake desolation') - connect(world, player, 'Eastern lake desolation', 'Upper lake desolation', lambda state: logic.has_fire(state) and state.can_reach('Upper Lake Serene', 'Region', player)) + connect(world, player, 'Eastern lake desolation', 'Upper lake desolation', lambda state: logic.has_fire(state) and state.can_reach('Upper Lake Serene', 'Region', player), "Upper Lake Serene") connect(world, player, 'Library', 'Eastern lake desolation') connect(world, player, 'Library', 'Library top', lambda state: logic.has_doublejump(state) or state.has('Talaria Attachment', player)) connect(world, player, 'Library', 'Varndagroth tower left', logic.has_keycard_D) @@ -185,7 +185,7 @@ def create_regions_and_locations(world: MultiWorld, player: int, precalculated_w if is_option_enabled(world, player, "GyreArchives"): connect(world, player, 'The lab (upper)', 'Ravenlord\'s Lair', lambda state: state.has('Merchant Crow', player)) connect(world, player, 'Ravenlord\'s Lair', 'The lab (upper)') - connect(world, player, 'Library top', 'Ifrit\'s Lair', lambda state: state.has('Kobo', player) and state.can_reach('Refugee Camp', 'Region', player)) + connect(world, player, 'Library top', 'Ifrit\'s Lair', lambda state: state.has('Kobo', player) and state.can_reach('Refugee Camp', 'Region', player), "Refugee Camp") connect(world, player, 'Ifrit\'s Lair', 'Library top') @@ -242,11 +242,19 @@ def connectStartingRegion(world: MultiWorld, player: int): def connect(world: MultiWorld, player: int, source: str, target: str, - rule: Optional[Callable[[CollectionState], bool]] = None): + rule: Optional[Callable[[CollectionState], bool]] = None, + indirect: str = ""): sourceRegion = world.get_region(source, player) targetRegion = world.get_region(target, player) - sourceRegion.connect(targetRegion, rule=rule) + entrance = sourceRegion.connect(targetRegion, rule=rule) + + if indirect: + indirectRegion = world.get_region(indirect, player) + if indirectRegion in world.indirect_connections: + world.indirect_connections[indirectRegion].add(entrance) + else: + world.indirect_connections[indirectRegion] = {entrance} def split_location_datas_per_region(locations: List[LocationData]) -> Dict[str, List[LocationData]]: From 3b9b9353b7e7588cb59c496fd295397dcbcdf9b6 Mon Sep 17 00:00:00 2001 From: Fabian Dill Date: Wed, 12 Jun 2024 15:34:46 +0200 Subject: [PATCH 4/7] WebHost: delete old docs files (#3503) --- WebHost.py | 1 + 1 file changed, 1 insertion(+) diff --git a/WebHost.py b/WebHost.py index afacd6288ec2..08ef3c430795 100644 --- a/WebHost.py +++ b/WebHost.py @@ -58,6 +58,7 @@ def create_ordered_tutorials_file() -> typing.List[typing.Dict[str, typing.Any]] worlds[game] = world base_target_path = Utils.local_path("WebHostLib", "static", "generated", "docs") + shutil.rmtree(base_target_path, ignore_errors=True) for game, world in worlds.items(): # copy files from world's docs folder to the generated folder target_path = os.path.join(base_target_path, game) From 2daccded365258633bd8ac268c5a58ae9ee31a43 Mon Sep 17 00:00:00 2001 From: Fabian Dill Date: Wed, 12 Jun 2024 15:35:51 +0200 Subject: [PATCH 5/7] Core: don't lock progression (#3501) --- Fill.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Fill.py b/Fill.py index d8147b2eac80..4967ff073601 100644 --- a/Fill.py +++ b/Fill.py @@ -483,15 +483,15 @@ def mark_for_locking(location: Location): if panic_method == "swap": fill_restrictive(multiworld, multiworld.state, defaultlocations, progitempool, swap=True, - on_place=mark_for_locking, name="Progression", single_player_placement=multiworld.players == 1) + name="Progression", single_player_placement=multiworld.players == 1) elif panic_method == "raise": fill_restrictive(multiworld, multiworld.state, defaultlocations, progitempool, swap=False, - on_place=mark_for_locking, name="Progression", single_player_placement=multiworld.players == 1) + name="Progression", single_player_placement=multiworld.players == 1) elif panic_method == "start_inventory": fill_restrictive(multiworld, multiworld.state, defaultlocations, progitempool, swap=False, allow_partial=True, - on_place=mark_for_locking, name="Progression", single_player_placement=multiworld.players == 1) + name="Progression", single_player_placement=multiworld.players == 1) if progitempool: for item in progitempool: logging.debug(f"Moved {item} to start_inventory to prevent fill failure.") From acf85eb9abb3de4863fe9350a6cdddee71c9be44 Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 12 Jun 2024 18:54:59 +0200 Subject: [PATCH 6/7] Speedups: remove dependency on c++ (#2796) * Speedups: remove dependency on c++ * Speedups: intset: handle malloc failing * Speedups: intset: fix corner case for int64 on 32bit systems original idea was to only use bucket->val if int(-1) # this is all 0xff... adding 1 results in 0, but it's not negative +# configure INTSET for player +cdef extern from *: + """ + #define INTSET_NAME ap_player_set + #define INTSET_TYPE uint32_t // has to match ap_player_t + """ + +# create INTSET for player +cdef extern from "intset.h": + """ + #undef INTSET_NAME + #undef INTSET_TYPE + """ + ctypedef struct ap_player_set: + pass + + ap_player_set* ap_player_set_new(size_t bucket_count) nogil + void ap_player_set_free(ap_player_set* set) nogil + bint ap_player_set_add(ap_player_set* set, ap_player_t val) nogil + bint ap_player_set_contains(ap_player_set* set, ap_player_t val) nogil + cdef struct LocationEntry: # layout is so that @@ -185,7 +206,7 @@ cdef class LocationStore: def find_item(self, slots: Set[int], seeked_item_id: int) -> Generator[Tuple[int, int, int, int, int], None, None]: cdef ap_id_t item = seeked_item_id cdef ap_player_t receiver - cdef std_set[ap_player_t] receivers + cdef ap_player_set* receivers cdef size_t slot_count = len(slots) if slot_count == 1: # specialized implementation for single slot @@ -197,13 +218,20 @@ cdef class LocationStore: yield entry.sender, entry.location, entry.item, entry.receiver, entry.flags elif slot_count: # generic implementation with lookup in set - for receiver in slots: - receivers.insert(receiver) - with nogil: - for entry in self.entries[:self.entry_count]: - if entry.item == item and receivers.count(entry.receiver): - with gil: - yield entry.sender, entry.location, entry.item, entry.receiver, entry.flags + receivers = ap_player_set_new(min(1023, slot_count)) # limit top level struct to 16KB + if not receivers: + raise MemoryError() + try: + for receiver in slots: + if not ap_player_set_add(receivers, receiver): + raise MemoryError() + with nogil: + for entry in self.entries[:self.entry_count]: + if entry.item == item and ap_player_set_contains(receivers, entry.receiver): + with gil: + yield entry.sender, entry.location, entry.item, entry.receiver, entry.flags + finally: + ap_player_set_free(receivers) def get_for_player(self, slot: int) -> Dict[int, Set[int]]: cdef ap_player_t receiver = slot diff --git a/_speedups.pyxbld b/_speedups.pyxbld index e1fe19b2efc6..974eaed03b6a 100644 --- a/_speedups.pyxbld +++ b/_speedups.pyxbld @@ -1,8 +1,10 @@ -# This file is required to get pyximport to work with C++. -# Switching from std::set to a pure C implementation is still on the table to simplify everything. +# This file is used when doing pyximport +import os def make_ext(modname, pyxfilename): from distutils.extension import Extension return Extension(name=modname, sources=[pyxfilename], - language='c++') + depends=["intset.h"], + include_dirs=[os.getcwd()], + language="c") diff --git a/intset.h b/intset.h new file mode 100644 index 000000000000..fac84fb6f890 --- /dev/null +++ b/intset.h @@ -0,0 +1,135 @@ +/* A specialized unordered_set implementation for literals, where bucket_count + * is defined at initialization rather than increased automatically. + */ +#include +#include +#include +#include + +#ifndef INTSET_NAME +#error "Please #define INTSET_NAME ... before including intset.h" +#endif + +#ifndef INTSET_TYPE +#error "Please #define INTSET_TYPE ... before including intset.h" +#endif + +/* macros to generate unique names from INTSET_NAME */ +#ifndef INTSET_CONCAT +#define INTSET_CONCAT_(a, b) a ## b +#define INTSET_CONCAT(a, b) INTSET_CONCAT_(a, b) +#define INTSET_FUNC_(a, b) INTSET_CONCAT(a, _ ## b) +#endif + +#define INTSET_FUNC(name) INTSET_FUNC_(INTSET_NAME, name) +#define INTSET_BUCKET INTSET_CONCAT(INTSET_NAME, Bucket) +#define INTSET_UNION INTSET_CONCAT(INTSET_NAME, Union) + +#if defined(_MSC_VER) +#pragma warning(push) +#pragma warning(disable : 4200) +#endif + + +typedef struct { + size_t count; + union INTSET_UNION { + INTSET_TYPE val; + INTSET_TYPE *data; + } v; +} INTSET_BUCKET; + +typedef struct { + size_t bucket_count; + INTSET_BUCKET buckets[]; +} INTSET_NAME; + +static INTSET_NAME *INTSET_FUNC(new)(size_t buckets) +{ + size_t i, size; + INTSET_NAME *set; + + if (buckets < 1) + buckets = 1; + if ((SIZE_MAX - sizeof(INTSET_NAME)) / sizeof(INTSET_BUCKET) < buckets) + return NULL; + size = sizeof(INTSET_NAME) + buckets * sizeof(INTSET_BUCKET); + set = (INTSET_NAME*)malloc(size); + if (!set) + return NULL; + memset(set, 0, size); /* gcc -fanalyzer does not understand this sets all buckets' count to 0 */ + for (i = 0; i < buckets; i++) { + set->buckets[i].count = 0; + } + set->bucket_count = buckets; + return set; +} + +static void INTSET_FUNC(free)(INTSET_NAME *set) +{ + size_t i; + if (!set) + return; + for (i = 0; i < set->bucket_count; i++) { + if (set->buckets[i].count > 1) + free(set->buckets[i].v.data); + } + free(set); +} + +static bool INTSET_FUNC(contains)(INTSET_NAME *set, INTSET_TYPE val) +{ + size_t i; + INTSET_BUCKET* bucket = &set->buckets[(size_t)val % set->bucket_count]; + if (bucket->count == 1) + return bucket->v.val == val; + for (i = 0; i < bucket->count; ++i) { + if (bucket->v.data[i] == val) + return true; + } + return false; +} + +static bool INTSET_FUNC(add)(INTSET_NAME *set, INTSET_TYPE val) +{ + INTSET_BUCKET* bucket; + + if (INTSET_FUNC(contains)(set, val)) + return true; /* ok */ + + bucket = &set->buckets[(size_t)val % set->bucket_count]; + if (bucket->count == 0) { + bucket->v.val = val; + bucket->count = 1; + } else if (bucket->count == 1) { + INTSET_TYPE old = bucket->v.val; + bucket->v.data = (INTSET_TYPE*)malloc(2 * sizeof(INTSET_TYPE)); + if (!bucket->v.data) { + bucket->v.val = old; + return false; /* error */ + } + bucket->v.data[0] = old; + bucket->v.data[1] = val; + bucket->count = 2; + } else { + size_t new_bucket_size; + INTSET_TYPE* new_bucket_data; + + new_bucket_size = (bucket->count + 1) * sizeof(INTSET_TYPE); + new_bucket_data = (INTSET_TYPE*)realloc(bucket->v.data, new_bucket_size); + if (!new_bucket_data) + return false; /* error */ + bucket->v.data = new_bucket_data; + bucket->v.data[bucket->count++] = val; + } + return true; /* success */ +} + + +#if defined(_MSC_VER) +#pragma warning(pop) +#endif + +#undef INTSET_FUNC +#undef INTSET_BUCKET +#undef INTSET_UNION diff --git a/test/cpp/CMakeLists.txt b/test/cpp/CMakeLists.txt new file mode 100644 index 000000000000..927b7494dac4 --- /dev/null +++ b/test/cpp/CMakeLists.txt @@ -0,0 +1,49 @@ +cmake_minimum_required(VERSION 3.5) +project(ap-cpp-tests) + +enable_testing() + +find_package(GTest REQUIRED) + +if (CMAKE_CXX_COMPILER_ID STREQUAL "MSVC") + add_definitions("/source-charset:utf-8") + set(CMAKE_CXX_FLAGS_DEBUG "/MTd") + set(CMAKE_CXX_FLAGS_RELEASE "/MT") +elseif (CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + # enable static analysis for gcc + add_compile_options(-fanalyzer -Werror) + # disable stuff that gets triggered by googletest + add_compile_options(-Wno-analyzer-malloc-leak) + # enable asan for gcc + add_compile_options(-fsanitize=address) + add_link_options(-fsanitize=address) +endif () + +add_executable(test_default) + +target_include_directories(test_default + PRIVATE + ${GTEST_INCLUDE_DIRS} +) + +target_link_libraries(test_default + ${GTEST_BOTH_LIBRARIES} +) + +add_test( + NAME test_default + COMMAND test_default +) + +set_property( + TEST test_default + PROPERTY ENVIRONMENT "ASAN_OPTIONS=allocator_may_return_null=1" +) + +file(GLOB ITEMS *) +foreach(item ${ITEMS}) + if(IS_DIRECTORY ${item} AND EXISTS ${item}/CMakeLists.txt) + message(${item}) + add_subdirectory(${item}) + endif() +endforeach() diff --git a/test/cpp/README.md b/test/cpp/README.md new file mode 100644 index 000000000000..792b9be77e72 --- /dev/null +++ b/test/cpp/README.md @@ -0,0 +1,32 @@ +# C++ tests + +Test framework for C and C++ code in AP. + +## Adding a Test + +### GoogleTest + +Adding GoogleTests is as simple as creating a directory with +* one or more `test_*.cpp` files that define tests using + [GoogleTest API](https://google.github.io/googletest/) +* a `CMakeLists.txt` that adds the .cpp files to `test_default` target using + [target_sources](https://cmake.org/cmake/help/latest/command/target_sources.html) + +### CTest + +If either GoogleTest is not suitable for the test or the build flags / sources / libraries are incompatible, +you can add another CTest to the project using add_target and add_test, similar to how it's done for `test_default`. + +## Running Tests + +* Install [CMake](https://cmake.org/). +* Build and/or install GoogleTest and make sure + [CMake can find it](https://cmake.org/cmake/help/latest/module/FindGTest.html), or + [create a parent `CMakeLists.txt` that fetches GoogleTest](https://google.github.io/googletest/quickstart-cmake.html). +* Enter the directory with the top-most `CMakeLists.txt` and run + ```sh + mkdir build + cmake -S . -B build/ -DCMAKE_BUILD_TYPE=Release + cmake --build build/ --config Release && \ + ctest --test-dir build/ -C Release --output-on-failure + ``` diff --git a/test/cpp/intset/CMakeLists.txt b/test/cpp/intset/CMakeLists.txt new file mode 100644 index 000000000000..175e0bd0b9e8 --- /dev/null +++ b/test/cpp/intset/CMakeLists.txt @@ -0,0 +1,4 @@ +target_sources(test_default + PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/test_intset.cpp +) diff --git a/test/cpp/intset/test_intset.cpp b/test/cpp/intset/test_intset.cpp new file mode 100644 index 000000000000..2f85bea960c4 --- /dev/null +++ b/test/cpp/intset/test_intset.cpp @@ -0,0 +1,105 @@ +#include +#include +#include + +// uint32Set +#define INTSET_NAME uint32Set +#define INTSET_TYPE uint32_t +#include "../../../intset.h" +#undef INTSET_NAME +#undef INTSET_TYPE + +// int64Set +#define INTSET_NAME int64Set +#define INTSET_TYPE int64_t +#include "../../../intset.h" + + +TEST(IntsetTest, ZeroBuckets) +{ + // trying to allocate with zero buckets has to either fail or be functioning + uint32Set *set = uint32Set_new(0); + if (!set) + return; // failed -> OK + + EXPECT_FALSE(uint32Set_contains(set, 1)); + EXPECT_TRUE(uint32Set_add(set, 1)); + EXPECT_TRUE(uint32Set_contains(set, 1)); + uint32Set_free(set); +} + +TEST(IntsetTest, Duplicate) +{ + // adding the same number again can't fail + uint32Set *set = uint32Set_new(2); + ASSERT_TRUE(set); + EXPECT_TRUE(uint32Set_add(set, 0)); + EXPECT_TRUE(uint32Set_add(set, 0)); + EXPECT_TRUE(uint32Set_contains(set, 0)); + uint32Set_free(set); +} + +TEST(IntsetTest, SetAllocFailure) +{ + // try to allocate 100TB of RAM, should fail and return NULL + if (sizeof(size_t) < 8) + GTEST_SKIP() << "Alloc error not testable on 32bit"; + int64Set *set = int64Set_new(6250000000000ULL); + EXPECT_FALSE(set); + int64Set_free(set); +} + +TEST(IntsetTest, SetAllocOverflow) +{ + // try to overflow argument passed to malloc + int64Set *set = int64Set_new(std::numeric_limits::max()); + EXPECT_FALSE(set); + int64Set_free(set); +} + +TEST(IntsetTest, NullFree) +{ + // free(NULL) should not try to free buckets + uint32Set_free(NULL); + int64Set_free(NULL); +} + +TEST(IntsetTest, BucketRealloc) +{ + // add a couple of values to the same bucket to test growing the bucket + uint32Set* set = uint32Set_new(1); + ASSERT_TRUE(set); + EXPECT_FALSE(uint32Set_contains(set, 0)); + EXPECT_TRUE(uint32Set_add(set, 0)); + EXPECT_TRUE(uint32Set_contains(set, 0)); + for (uint32_t i = 1; i < 32; ++i) { + EXPECT_TRUE(uint32Set_add(set, i)); + EXPECT_TRUE(uint32Set_contains(set, i - 1)); + EXPECT_TRUE(uint32Set_contains(set, i)); + EXPECT_FALSE(uint32Set_contains(set, i + 1)); + } + uint32Set_free(set); +} + +TEST(IntSet, Max) +{ + constexpr auto n = std::numeric_limits::max(); + uint32Set *set = uint32Set_new(1); + ASSERT_TRUE(set); + EXPECT_FALSE(uint32Set_contains(set, n)); + EXPECT_TRUE(uint32Set_add(set, n)); + EXPECT_TRUE(uint32Set_contains(set, n)); + uint32Set_free(set); +} + +TEST(InsetTest, Negative) +{ + constexpr auto n = std::numeric_limits::min(); + static_assert(n < 0, "n not negative"); + int64Set *set = int64Set_new(3); + ASSERT_TRUE(set); + EXPECT_FALSE(int64Set_contains(set, n)); + EXPECT_TRUE(int64Set_add(set, n)); + EXPECT_TRUE(int64Set_contains(set, n)); + int64Set_free(set); +} diff --git a/test/netutils/test_location_store.py b/test/netutils/test_location_store.py index a7f117255faa..f3e83989bea4 100644 --- a/test/netutils/test_location_store.py +++ b/test/netutils/test_location_store.py @@ -1,4 +1,5 @@ # Tests for _speedups.LocationStore and NetUtils._LocationStore +import os import typing import unittest import warnings @@ -7,6 +8,8 @@ State = typing.Dict[typing.Tuple[int, int], typing.Set[int]] RawLocations = typing.Dict[int, typing.Dict[int, typing.Tuple[int, int, int]]] +ci = bool(os.environ.get("CI")) # always set in GitHub actions + sample_data: RawLocations = { 1: { 11: (21, 2, 7), @@ -24,6 +27,9 @@ 3: { 9: (99, 4, 0), }, + 5: { + 9: (99, 5, 0), + } } empty_state: State = { @@ -45,14 +51,14 @@ class TestLocationStore(unittest.TestCase): store: typing.Union[LocationStore, _LocationStore] def test_len(self) -> None: - self.assertEqual(len(self.store), 4) + self.assertEqual(len(self.store), 5) self.assertEqual(len(self.store[1]), 3) def test_key_error(self) -> None: with self.assertRaises(KeyError): _ = self.store[0] with self.assertRaises(KeyError): - _ = self.store[5] + _ = self.store[6] locations = self.store[1] # no Exception with self.assertRaises(KeyError): _ = locations[7] @@ -71,7 +77,7 @@ def test_get(self) -> None: self.assertEqual(self.store[1].get(10, (None, None, None)), (None, None, None)) def test_iter(self) -> None: - self.assertEqual(sorted(self.store), [1, 2, 3, 4]) + self.assertEqual(sorted(self.store), [1, 2, 3, 4, 5]) self.assertEqual(len(self.store), len(sample_data)) self.assertEqual(list(self.store[1]), [11, 12, 13]) self.assertEqual(len(self.store[1]), len(sample_data[1])) @@ -85,13 +91,26 @@ def test_items(self) -> None: self.assertEqual(sorted(self.store[1].items())[0][1], self.store[1][11]) def test_find_item(self) -> None: + # empty player set self.assertEqual(sorted(self.store.find_item(set(), 99)), []) + # no such player, single + self.assertEqual(sorted(self.store.find_item({6}, 99)), []) + # no such player, set + self.assertEqual(sorted(self.store.find_item({7, 8, 9}, 99)), []) + # no such item self.assertEqual(sorted(self.store.find_item({3}, 1)), []) - self.assertEqual(sorted(self.store.find_item({5}, 99)), []) + # valid matches self.assertEqual(sorted(self.store.find_item({3}, 99)), [(4, 9, 99, 3, 0)]) self.assertEqual(sorted(self.store.find_item({3, 4}, 99)), [(3, 9, 99, 4, 0), (4, 9, 99, 3, 0)]) + self.assertEqual(sorted(self.store.find_item({2, 3, 4}, 99)), + [(3, 9, 99, 4, 0), (4, 9, 99, 3, 0)]) + # test hash collision in set + self.assertEqual(sorted(self.store.find_item({3, 5}, 99)), + [(4, 9, 99, 3, 0), (5, 9, 99, 5, 0)]) + self.assertEqual(sorted(self.store.find_item(set(range(2048)), 13)), + [(1, 13, 13, 1, 0)]) def test_get_for_player(self) -> None: self.assertEqual(self.store.get_for_player(3), {4: {9}}) @@ -196,18 +215,20 @@ def setUp(self) -> None: super().setUp() -@unittest.skipIf(LocationStore is _LocationStore, "_speedups not available") +@unittest.skipIf(LocationStore is _LocationStore and not ci, "_speedups not available") class TestSpeedupsLocationStore(Base.TestLocationStore): """Run base method tests for cython implementation.""" def setUp(self) -> None: + self.assertFalse(LocationStore is _LocationStore, "Failed to load _speedups") self.store = LocationStore(sample_data) super().setUp() -@unittest.skipIf(LocationStore is _LocationStore, "_speedups not available") +@unittest.skipIf(LocationStore is _LocationStore and not ci, "_speedups not available") class TestSpeedupsLocationStoreConstructor(Base.TestLocationStoreConstructor): """Run base constructor tests and tests the additional constraints for cython implementation.""" def setUp(self) -> None: + self.assertFalse(LocationStore is _LocationStore, "Failed to load _speedups") self.type = LocationStore super().setUp() From c108845d1ff26979ddb4a5168faec82b1206292e Mon Sep 17 00:00:00 2001 From: black-sliver <59490463+black-sliver@users.noreply.github.com> Date: Wed, 12 Jun 2024 18:55:48 +0200 Subject: [PATCH 7/7] CI: more checks in build and rework compression (#3336) * CI: build: fail fast if setup.py fails on windows * CI: build: fail for missing uploads, rework compression Upload-artifact allows setting compression level now. The change speeds up both upload and extraction. * CI: match build gz in release * CI: build: verify worlds all load * CI: build: generate a game * Generate: move worlds loaded exception to allow settings to init from worlds * CI: build: build setup before running tests --- .github/workflows/build.yml | 60 +++++++++++++++++++++++++++++++---- .github/workflows/release.yml | 2 +- Generate.py | 8 +++-- 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 80aaf70c215e..dd88d8d7d7bf 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -40,6 +40,10 @@ jobs: run: | python -m pip install --upgrade pip python setup.py build_exe --yes + if ( $? -eq $false ) { + Write-Error "setup.py failed!" + exit 1 + } $NAME="$(ls build | Select-String -Pattern 'exe')".Split('.',2)[1] $ZIP_NAME="Archipelago_$NAME.7z" echo "$NAME -> $ZIP_NAME" @@ -49,12 +53,6 @@ jobs: Rename-Item "exe.$NAME" Archipelago 7z a -mx=9 -mhe=on -ms "../dist/$ZIP_NAME" Archipelago Rename-Item Archipelago "exe.$NAME" # inno_setup.iss expects the original name - - name: Store 7z - uses: actions/upload-artifact@v4 - with: - name: ${{ env.ZIP_NAME }} - path: dist/${{ env.ZIP_NAME }} - retention-days: 7 # keep for 7 days, should be enough - name: Build Setup run: | & "${env:ProgramFiles(x86)}\Inno Setup 6\iscc.exe" inno_setup.iss /DNO_SIGNTOOL @@ -65,11 +63,38 @@ jobs: $contents = Get-ChildItem -Path setups/*.exe -Force -Recurse $SETUP_NAME=$contents[0].Name echo "SETUP_NAME=$SETUP_NAME" >> $Env:GITHUB_ENV + - name: Check build loads expected worlds + shell: bash + run: | + cd build/exe* + mv Players/Templates/meta.yaml . + ls -1 Players/Templates | sort > setup-player-templates.txt + rm -R Players/Templates + timeout 30 ./ArchipelagoLauncher "Generate Template Options" || true + ls -1 Players/Templates | sort > generated-player-templates.txt + cmp setup-player-templates.txt generated-player-templates.txt \ + || diff setup-player-templates.txt generated-player-templates.txt + mv meta.yaml Players/Templates/ + - name: Test Generate + shell: bash + run: | + cd build/exe* + cp Players/Templates/Clique.yaml Players/ + timeout 30 ./ArchipelagoGenerate + - name: Store 7z + uses: actions/upload-artifact@v4 + with: + name: ${{ env.ZIP_NAME }} + path: dist/${{ env.ZIP_NAME }} + compression-level: 0 # .7z is incompressible by zip + if-no-files-found: error + retention-days: 7 # keep for 7 days, should be enough - name: Store Setup uses: actions/upload-artifact@v4 with: name: ${{ env.SETUP_NAME }} path: setups/${{ env.SETUP_NAME }} + if-no-files-found: error retention-days: 7 # keep for 7 days, should be enough build-ubuntu2004: @@ -110,7 +135,7 @@ jobs: echo -e "setup.py dist output:\n `ls dist`" cd dist && export APPIMAGE_NAME="`ls *.AppImage`" && cd .. export TAR_NAME="${APPIMAGE_NAME%.AppImage}.tar.gz" - (cd build && DIR_NAME="`ls | grep exe`" && mv "$DIR_NAME" Archipelago && tar -czvf ../dist/$TAR_NAME Archipelago && mv Archipelago "$DIR_NAME") + (cd build && DIR_NAME="`ls | grep exe`" && mv "$DIR_NAME" Archipelago && tar -cv Archipelago | gzip -8 > ../dist/$TAR_NAME && mv Archipelago "$DIR_NAME") echo "APPIMAGE_NAME=$APPIMAGE_NAME" >> $GITHUB_ENV echo "TAR_NAME=$TAR_NAME" >> $GITHUB_ENV # - copy code above to release.yml - @@ -118,15 +143,36 @@ jobs: run: | source venv/bin/activate python setup.py build_exe --yes + - name: Check build loads expected worlds + shell: bash + run: | + cd build/exe* + mv Players/Templates/meta.yaml . + ls -1 Players/Templates | sort > setup-player-templates.txt + rm -R Players/Templates + timeout 30 ./ArchipelagoLauncher "Generate Template Options" || true + ls -1 Players/Templates | sort > generated-player-templates.txt + cmp setup-player-templates.txt generated-player-templates.txt \ + || diff setup-player-templates.txt generated-player-templates.txt + mv meta.yaml Players/Templates/ + - name: Test Generate + shell: bash + run: | + cd build/exe* + cp Players/Templates/Clique.yaml Players/ + timeout 30 ./ArchipelagoGenerate - name: Store AppImage uses: actions/upload-artifact@v4 with: name: ${{ env.APPIMAGE_NAME }} path: dist/${{ env.APPIMAGE_NAME }} + if-no-files-found: error retention-days: 7 - name: Store .tar.gz uses: actions/upload-artifact@v4 with: name: ${{ env.TAR_NAME }} path: dist/${{ env.TAR_NAME }} + compression-level: 0 # .gz is incompressible by zip + if-no-files-found: error retention-days: 7 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 2d7f1253b760..3f8651d408e7 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -69,7 +69,7 @@ jobs: echo -e "setup.py dist output:\n `ls dist`" cd dist && export APPIMAGE_NAME="`ls *.AppImage`" && cd .. export TAR_NAME="${APPIMAGE_NAME%.AppImage}.tar.gz" - (cd build && DIR_NAME="`ls | grep exe`" && mv "$DIR_NAME" Archipelago && tar -czvf ../dist/$TAR_NAME Archipelago && mv Archipelago "$DIR_NAME") + (cd build && DIR_NAME="`ls | grep exe`" && mv "$DIR_NAME" Archipelago && tar -cv Archipelago | gzip -8 > ../dist/$TAR_NAME && mv Archipelago "$DIR_NAME") echo "APPIMAGE_NAME=$APPIMAGE_NAME" >> $GITHUB_ENV echo "TAR_NAME=$TAR_NAME" >> $GITHUB_ENV # - code above copied from build.yml - diff --git a/Generate.py b/Generate.py index 0cef081120e6..1fbb9e76a483 100644 --- a/Generate.py +++ b/Generate.py @@ -66,13 +66,15 @@ def get_seed_name(random_source) -> str: def main(args=None): + # __name__ == "__main__" check so unittests that already imported worlds don't trip this. + if __name__ == "__main__" and "worlds" in sys.modules: + raise Exception("Worlds system should not be loaded before logging init.") + if not args: args = mystery_argparse() seed = get_seed(args.seed) - # __name__ == "__main__" check so unittests that already imported worlds don't trip this. - if __name__ == "__main__" and "worlds" in sys.modules: - raise Exception("Worlds system should not be loaded before logging init.") + Utils.init_logging(f"Generate_{seed}", loglevel=args.log_level) random.seed(seed) seed_name = get_seed_name(random)