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.

Sign up for GitHub

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

Jump to bottom

Fixing and cleaning up configs #40

Merged
merged 14 commits into from
Dec 1, 2023
Merged

Fixing and cleaning up configs #40

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
86a0b52
Cleanup
dima-dencep Nov 19, 2023
ec66244
Allow disabling config watcher
dima-dencep Nov 19, 2023
27a4dba
Allow seting of a custom config handler
dima-dencep Nov 19, 2023
8f43edf
Allow events to be fired from ModConfig
dima-dencep Nov 19, 2023
a983ef1
Try fix ParsingException
dima-dencep Nov 19, 2023
7757869
Allows to set a custom number of backups
dima-dencep Nov 19, 2023
21d58e4
Revert "Allows to set a custom number of backups"
dima-dencep Nov 23, 2023
ae5316b
Remove `configHandler`
dima-dencep Nov 23, 2023
005dbec
Revert "Allow events to be fired from ModConfig"
dima-dencep Nov 23, 2023
5bd10be
Cast to FileConfig instead of CommentedFileConfig
dima-dencep Nov 23, 2023
3dd9a83
Make `TOML` final
dima-dencep Nov 23, 2023
efc0bf6
Remove `configHandler` from ModContainer
dima-dencep Nov 23, 2023
cae2ac0
Remove `fireEvent`
dima-dencep Nov 23, 2023
3b9d8f1
Merge branch 'neoforged:main' into main
dima-dencep Nov 30, 2023
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
67 changes: 45 additions & 22 deletions core/src/main/java/net/neoforged/fml/config/ConfigFileTypeHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,38 +31,56 @@ public class ConfigFileTypeHandler {
public Function<ModConfig, CommentedFileConfig> reader(Path configBasePath) {
return (c) -> {
final Path configPath = configBasePath.resolve(c.getFileName());
final CommentedFileConfig configData = CommentedFileConfig.builder(configPath).sync().
preserveInsertionOrder().
autosave().
onFileNotFound((newfile, configFormat)-> setupConfigFile(c, newfile, configFormat)).
writingMode(WritingMode.REPLACE).
build();
LOGGER.debug(CONFIG, "Built TOML config for {}", configPath.toString());
final CommentedFileConfig configData = CommentedFileConfig.builder(configPath)
.sync()
Technici4n marked this conversation as resolved.
Show resolved Hide resolved
.preserveInsertionOrder()
.autosave()
.onFileNotFound((newfile, configFormat) -> setupConfigFile(c, newfile, configFormat))
.writingMode(WritingMode.REPLACE)
.build();
LOGGER.debug(CONFIG, "Built TOML config for {}", configPath);
try
{
configData.load();
}
catch (ParsingException ex)
{
throw new ConfigLoadingException(c, ex);
LOGGER.warn(CONFIG, "Attempting to recreate {}", configPath);
try
{
backUpConfig(configData.getNioPath(), FMLConfig.getIntConfigValue(FMLConfig.ConfigValue.CONFIG_BACKUPS_COUNT));
Files.delete(configData.getNioPath());

configData.load();
}
catch (Throwable t)
{
ex.addSuppressed(t);

throw new ConfigLoadingException(c, ex);
}
Technici4n marked this conversation as resolved.
Show resolved Hide resolved
}
LOGGER.debug(CONFIG, "Loaded TOML config file {}", configPath.toString());
try {
FileWatcher.defaultInstance().addWatch(configPath, new ConfigWatcher(c, configData, Thread.currentThread().getContextClassLoader()));
LOGGER.debug(CONFIG, "Watching TOML config file {} for changes", configPath.toString());
} catch (IOException e) {
throw new RuntimeException("Couldn't watch config file", e);
LOGGER.debug(CONFIG, "Loaded TOML config file {}", configPath);
if (!FMLConfig.getBoolConfigValue(FMLConfig.ConfigValue.DISABLE_CONFIG_WATCHER)) {
try {
FileWatcher.defaultInstance().addWatch(configPath, new ConfigWatcher(c, configData, Thread.currentThread().getContextClassLoader()));
LOGGER.debug(CONFIG, "Watching TOML config file {} for changes", configPath);
} catch (IOException e) {
throw new RuntimeException("Couldn't watch config file", e);
}
}
return configData;
};
}

public void unload(Path configBasePath, ModConfig config) {
if (FMLConfig.getBoolConfigValue(FMLConfig.ConfigValue.DISABLE_CONFIG_WATCHER))
return;
Path configPath = configBasePath.resolve(config.getFileName());
try {
FileWatcher.defaultInstance().removeWatch(configBasePath.resolve(config.getFileName()));
FileWatcher.defaultInstance().removeWatch(configPath);
} catch (RuntimeException e) {
LOGGER.error("Failed to remove config {} from tracker!", configPath.toString(), e);
LOGGER.error("Failed to remove config {} from tracker!", configPath, e);
}
}

Expand All @@ -81,14 +99,19 @@ private boolean setupConfigFile(final ModConfig modConfig, final Path file, fina

public static void backUpConfig(final CommentedFileConfig commentedFileConfig)
{
backUpConfig(commentedFileConfig, 5); //TODO: Think of a way for mods to set their own preference (include a sanity check as well, no disk stuffing)
backUpConfig(commentedFileConfig, FMLConfig.getIntConfigValue(FMLConfig.ConfigValue.CONFIG_BACKUPS_COUNT));
}

public static void backUpConfig(final CommentedFileConfig commentedFileConfig, final int maxBackups)
{
Path bakFileLocation = commentedFileConfig.getNioPath().getParent();
String bakFileName = FilenameUtils.removeExtension(commentedFileConfig.getFile().getName());
String bakFileExtension = FilenameUtils.getExtension(commentedFileConfig.getFile().getName()) + ".bak";
backUpConfig(commentedFileConfig.getNioPath(), maxBackups);
}

public static void backUpConfig(final Path commentedFileConfig, final int maxBackups)
{
Path bakFileLocation = commentedFileConfig.getParent();
String bakFileName = FilenameUtils.removeExtension(commentedFileConfig.getFileName().toString());
String bakFileExtension = FilenameUtils.getExtension(commentedFileConfig.getFileName().toString()) + ".bak";
Path bakFile = bakFileLocation.resolve(bakFileName + "-1" + "." + bakFileExtension);
try
{
Expand All @@ -103,11 +126,11 @@ public static void backUpConfig(final CommentedFileConfig commentedFileConfig, f
Files.move(oldBak, bakFileLocation.resolve(bakFileName + "-" + (i + 1) + "." + bakFileExtension));
}
}
Files.copy(commentedFileConfig.getNioPath(), bakFile);
Files.copy(commentedFileConfig, bakFile);
}
catch (IOException exception)
{
LOGGER.warn(CONFIG, "Failed to back up config file {}", commentedFileConfig.getNioPath(), exception);
LOGGER.warn(CONFIG, "Failed to back up config file {}", commentedFileConfig, exception);
}
}

Expand Down
12 changes: 7 additions & 5 deletions core/src/main/java/net/neoforged/fml/config/ModConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import java.io.ByteArrayInputStream;
import java.nio.file.Path;
import java.util.Locale;
import java.util.concurrent.Callable;

public class ModConfig
{
Expand All @@ -24,17 +23,20 @@ public class ModConfig
private final ModContainer container;
private final ConfigFileTypeHandler configHandler;
private CommentedConfig configData;
private Callable<Void> saveHandler;

public ModConfig(final Type type, final IConfigSpec<?> spec, final ModContainer container, final String fileName) {
public ModConfig(final Type type, final IConfigSpec<?> spec, final ModContainer container, final String fileName, ConfigFileTypeHandler configHandler) {
dima-dencep marked this conversation as resolved.
Show resolved Hide resolved
this.type = type;
this.spec = spec;
this.fileName = fileName;
this.container = container;
this.configHandler = ConfigFileTypeHandler.TOML;
this.configHandler = configHandler;
ConfigTracker.INSTANCE.trackConfig(this);
}

public ModConfig(final Type type, final IConfigSpec<?> spec, final ModContainer container, final String fileName) {
this(type, spec, container, fileName, ConfigFileTypeHandler.TOML);
}

public ModConfig(final Type type, final IConfigSpec<?> spec, final ModContainer activeContainer) {
this(type, spec, activeContainer, defaultConfigName(type, activeContainer.getModId()));
}
Expand Down Expand Up @@ -73,7 +75,7 @@ void setConfigData(final CommentedConfig configData) {
this.spec.acceptConfig(this.configData);
}

void fireEvent(final IConfigEvent configEvent) {
public void fireEvent(final IConfigEvent configEvent) {
dima-dencep marked this conversation as resolved.
Show resolved Hide resolved
this.container.dispatchConfigEvent(configEvent);
}

Expand Down
2 changes: 2 additions & 0 deletions loader/src/main/java/net/neoforged/fml/loading/FMLConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,12 @@
public class FMLConfig
{
public enum ConfigValue {
DISABLE_CONFIG_WATCHER("disableConfigWatcher", Boolean.FALSE, "Disables File Watcher. Used to automatically update config if its file has been modified."),
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of disabling the config watcher? If it is to fix a bug, it might already be fixed in upstream NightConfig. Or at least it was reported to TheElectronWill some time ago and he said a fix was in the pipeline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically, there's no point as such, but I'd like to be able to do so

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I'm in favor of this change - there are outstanding issues with the watcher on our end that are not addressed, such as the Loading & Reloading events being able to fire simultaneously, and the whole concept being quite broken with editors that don't save atomically. So I think giving users the option to disable the watcher is a good stopgap until someone has time to properly rewrite that logic.

Copy link
Member

Choose a reason for hiding this comment

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

Should we maybe disable it by default then for now? (at least in production)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about a complete remove?

Copy link
Member

Choose a reason for hiding this comment

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

No - it would be nice to fix it eventually so please leave it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, what else do I need to do to get this pr merged? (I can't continue working on one mod without these changes)

EARLY_WINDOW_CONTROL("earlyWindowControl", Boolean.TRUE, "Should we control the window. Disabling this disables new GL features and can be bad for mods that rely on them."),
MAX_THREADS("maxThreads", -1, "Max threads for early initialization parallelism, -1 is based on processor count", FMLConfig::maxThreads),
VERSION_CHECK("versionCheck", Boolean.TRUE, "Enable NeoForge global version checking"),
DEFAULT_CONFIG_PATH("defaultConfigPath", "defaultconfigs", "Default config path for servers"),
CONFIG_BACKUPS_COUNT("configBackupsCount", 5, "Sets the number of backups for configs."),
dima-dencep marked this conversation as resolved.
Show resolved Hide resolved
DISABLE_OPTIMIZED_DFU("disableOptimizedDFU", Boolean.TRUE, "Disables Optimized DFU client-side - already disabled on servers"),
EARLY_WINDOW_PROVIDER("earlyWindowProvider", "fmlearlywindow", "Early window provider"),
EARLY_WINDOW_WIDTH("earlyWindowWidth", 854, "Early window width"),
Expand Down